Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix selector for table-layout rule #1761

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Apr 8, 2024

Fixes #1063 (again, issue reopened).

I noticed a CSS selector that looks like it's missing a comma.

This CSS rule has some history:

@drammock
Copy link
Collaborator

drammock commented Apr 9, 2024

gotta love those one-character PRs!

Copy link
Collaborator

@Carreau Carreau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could/should we have a scss autoformatter to make those issues more visible ? I doubt a linter can detect that though.

@trallard
Copy link
Collaborator

We are using prettier as a pre-commit hook. Though reading through https://prettier.io/docs/en/comparison we might need a combination of prettier + a linter to catch these type of bugs

@drammock
Copy link
Collaborator

@gabalafou
Copy link
Collaborator Author

gabalafou commented Apr 11, 2024

I added a linter in another PR (#1764) but as @Carreau guessed, it does not pick up the error fixed in this PR.

I wish I knew a good way to add a regression test. But I don't even know how to recreate the original issue with a data table on which I vary table-layout from "auto" to "fixed". It seems like something else was going on #1063 and #1017.

I actually don't think the root issue originally was just the table-layout property, though. As far as I can tell, the table-layout: fixed rule only becomes an issue when the table's width is constrained, for example by a CSS width rule on the table element itself.

So I think that issues #1063 and #1017 were actually fixed in #1482 by a change in _tables.scss.

In fact I wonder if the right thing to do would be to remove the table-layout rule. Unless an explicit width is set, table-layout: fixed doesn't actually have an effect.

The reason why we're even dealing with table-layout: fixed is because nbsphinx added it to their stylesheet (spatialaudio/nbsphinx#228), and the only reason nbsphinx added it is because Jupyter Notebook added it. There's some discussion on jupyter/notebook#1776 about whether to use table-layout auto or fixed, but it's a little unclear to me what the ultimate takeaways from that discussion are.

@gabalafou gabalafou marked this pull request as draft April 11, 2024 01:54
@gabalafou
Copy link
Collaborator Author

gabalafou commented Apr 24, 2024

Thinking about this some more, I think we can apply table-layout: auto and call it a day. This seems to me the safest option. Removing the rule could cause the fixed rule to apply in some edge cases where for some reason an explicit width is set on a table (and fixed layout is generally more problematic, you should only use it when you really know ahead of time what the table contains, where it will appear, how much width it will have).

@gabalafou gabalafou marked this pull request as ready for review April 24, 2024 13:34
@gabalafou
Copy link
Collaborator Author

I marked this ready for review

@drammock drammock merged commit a78a066 into pydata:main Apr 24, 2024
34 checks passed
@drammock
Copy link
Collaborator

thanks for the detective work @gabalafou

@gabalafou gabalafou deleted the fix-table-layout-selector branch April 24, 2024 14:33
gabalafou added a commit to gabalafou/pydata-sphinx-theme that referenced this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inproper styling of dataframes for nbsphinx + pydata theme when contained in ipywidget
4 participants