pydata / pydata-sphinx-theme

A clean, three-column Sphinx theme with Bootstrap for the PyData community
https://pydata-sphinx-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
600 stars 313 forks source link

Fix selector for table-layout rule #1761

Closed gabalafou closed 5 months ago

gabalafou commented 5 months ago

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 commented 5 months ago

gotta love those one-character PRs!

trallard commented 5 months ago

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 commented 5 months ago

a quick search turned up

gabalafou commented 5 months ago

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 (https://github.com/spatialaudio/nbsphinx/pull/228), and the only reason nbsphinx added it is because Jupyter Notebook added it. There's some discussion on https://github.com/jupyter/notebook/pull/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 commented 5 months ago

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 commented 5 months ago

I marked this ready for review

drammock commented 5 months ago

thanks for the detective work @gabalafou