Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upFix table regressions. #9306
Conversation
jasmussen
added
[Type] Bug
Regression
labels
Aug 24, 2018
jasmussen
added this to the 3.7 milestone
Aug 24, 2018
jasmussen
self-assigned this
Aug 24, 2018
jasmussen
requested review from
earnjam
and
WordPress/gutenberg-core
Aug 24, 2018
ellatrix
reviewed
Aug 24, 2018
| @@ -1,5 +1,6 @@ | |||
| .wp-block-table { | |||
| overflow-x: auto; | |||
| display: block; // Necessary in order for this table to be mobile responsive. | |||
This comment has been minimized.
This comment has been minimized.
ellatrix
Aug 24, 2018
Member
This seems odd to me, changing display of a table element. I had some problem with this rule in a PR, so I took it out. What Exactly does mobile responsive mean here? Could you elaborate?
This comment has been minimized.
This comment has been minimized.
jasmussen
Aug 30, 2018
Author
Contributor
To answer your question, if we had the whole thing wrapped in a div, we could do this instead:
https://codepen.io/joen/pen/RYojVZ?editors=1100
But as the markup is today, we need to leverage the table as the full-width block to hold the horizontal scrollbar, and then make a child element of that (in this case, tbody/thead/tfoot) be the full-wide table that has a min-width.
ellatrix
reviewed
Aug 24, 2018
| @@ -1,6 +1,7 @@ | |||
| .wp-block-table { | |||
| // Fixed layout toggle | |||
| &.has-fixed-layout tbody { | |||
| display: table; | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ellatrix
reviewed
Aug 24, 2018
| @@ -1,6 +1,7 @@ | |||
| .wp-block-table { | |||
| // Fixed layout toggle | |||
| &.has-fixed-layout tbody { | |||
| display: table; | |||
| table-layout: fixed; | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Wondering if there could be any alternative fix for unresponsiveness. I'm guessing tables are by nature unresponsive though... If you have a few columns, you'll want to scroll them, not have them all squashed together. I wouldn't mess with the normal CSS rules of a table to make it more responsive. Could it help to wrap the table in a (scrollable) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
All good feedback, thank you. As a preface, there are two features of the table block being discussed. One is the fixed layout, which is off by default and toggled on in the sidebar. The other is the responsiveness, which, note, is stored in the But the idea here is that every block should come with a minimum level of mobile responsiveness. And a "true" responsive table isn't really a table anymore, it's a rather complex collection of divs put together to look like a table. This is completely fair to explores separately, but as the vanilla offering we have to consider the basics which are accessible. For that reason, the most minimal responsiveness I could find, was to have the table scroll horizontally, if less than 360px wide. You can see that here: https://codepen.io/joen/pen/RYojVZ Due to the nature of how the Here's a GIF: If we can accomplish a minimally responsive table in a different way, I'd love suggestions. |
jasmussen
requested a review
from WordPress/gutenberg-core
Aug 30, 2018
This comment has been minimized.
This comment has been minimized.
|
I think the thead and tfoot should also be added. I feel a bit uncomfortable with changing the display of a table from the default, but if this is valid and the way to do it, ok. Is there any way tests could be added for this (unsure here)? This seems like something with so many different configurations that we could easily break in the future. |
This comment has been minimized.
This comment has been minimized.
|
I could use some help getting this PR in ship shape if you have bandwidth. I feel strongly that all blocks we ship should have some measure of responsiveness, remember it's opt-in for themes. Right now both the responsiveness and the fixed-cell-width are broken in master, so it would be nice to find some solution soon, even if we need to refine it later. |
This comment has been minimized.
This comment has been minimized.
|
@jasmussen Cool, I'm fine with just restoring the display rule. Could we just add |
This comment has been minimized.
This comment has been minimized.
|
Will do, feel free to separately track any concerns you have and we'll look at it. |
This comment has been minimized.
This comment has been minimized.
|
Addressed the feedback. Thanks for the reviews. If we need to refactor the table display values to not re-do the tbody/tfoot/thead elements, we need a wrapping div, see https://codepen.io/joen/pen/RYojVZ?editors=1100 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hmm, you're right. And this suggests @iseulde was right to be nervous as well, because what causes the table to collapse like that is the changing to I pushed a fix in b7d41bf, and I do think we should merge this because it fixes a regression — it is frustrating that the fixed width switch is broken in master. But if we discover that changing the display properties of the table, thead, tbody, and tfoot, we should look at an alternative fix, which I think would include a wrapping div, see: https://codepen.io/joen/pen/RYojVZ. What do you think? Should we get this merged so we can fix the regression? |
This comment has been minimized.
This comment has been minimized.
|
Oh, taking a look at the inverted styles in the Twenty theme. |
This comment has been minimized.
This comment has been minimized.
ellatrix
approved these changes
Aug 30, 2018
This comment has been minimized.
This comment has been minimized.
|
Thanks, let's get this in and look at any other enhancements separately. |
jasmussen
merged commit 07eeecf
into
master
Aug 30, 2018
1 check passed
jasmussen
deleted the
try/table-fix
branch
Aug 30, 2018
This comment has been minimized.
This comment has been minimized.
It looks like this is the cause of #9779: thead, tbody and tfoot need to be set to their default style properties for columns to properly align. We then also have the secondary issue that the table needs to be Unfortunately it looks like we need another approach. Probably a wrapping Seeing as we're discussing it on #9801, I think I'll introduce a fix there. That PR also handily introduces a block deprecation, so probably makes sense to handle it there. |








jasmussen commentedAug 24, 2018
This PR fixes table block regressions introduced in #7899 (comment)
The block property for the table makes sure that it's mobile responsive. The "display table" is necessary for fixed layout to work.
display: table;ontbodyis necessary for thehas-fixed-layoutoption to work. Please see #6314 for a GIFdisplay: blockon.wp-block-tableis necessary for the table itself to be responsive. Please see #2152.Note that the responsiveness for the table is in the
theme.scssfile, which means themes have to opt into these "opinionated styles".Please test this PR with and without the fixed table cells option enabled, frontend and backend. And please verify that this PR does not regress what #7899 was meant to fix.