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 table regressions. #9306

Merged
merged 4 commits into from Aug 30, 2018

Conversation

Projects
None yet
5 participants
@jasmussen
Contributor

jasmussen commented Aug 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; on tbody is necessary for the has-fixed-layout option to work. Please see #6314 for a GIF
  • display: block on .wp-block-table is necessary for the table itself to be responsive. Please see #2152.

Note that the responsiveness for the table is in the theme.scss file, 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.

Fix table regressions.
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.

@jasmussen jasmussen added this to the 3.7 milestone Aug 24, 2018

@jasmussen jasmussen self-assigned this Aug 24, 2018

@jasmussen jasmussen requested review from earnjam and WordPress/gutenberg-core 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.

@iseulde

iseulde 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?

@iseulde

iseulde 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.

@jasmussen

jasmussen Aug 30, 2018

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.

@jasmussen

jasmussen Aug 30, 2018

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.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 24, 2018

Member

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) figure tag or something?

Member

iseulde commented Aug 24, 2018

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) figure tag or something?

@earnjam

This comment has been minimized.

Show comment
Hide comment
@earnjam

earnjam Aug 24, 2018

Contributor

I think I had found a fix to this issue, but was holding off because I wasn't sure if everything was going to be redone as part of #6923. I'll see if I can find it on a local branch and compare to what @iseulde merged in #8767.

Contributor

earnjam commented Aug 24, 2018

I think I had found a fix to this issue, but was holding off because I wasn't sure if everything was going to be redone as part of #6923. I'll see if I can find it on a local branch and compare to what @iseulde merged in #8767.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 27, 2018

Contributor

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 theme.scss file and therefore something a theme has to opt into, to receive.

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 display: table; property works, though, this involves some trickery to make the full width behave more like a div than a table, because the table will always try to contain its contents.

Here's a GIF:

responsive table

If we can accomplish a minimally responsive table in a different way, I'd love suggestions.

Contributor

jasmussen commented Aug 27, 2018

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 theme.scss file and therefore something a theme has to opt into, to receive.

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 display: table; property works, though, this involves some trickery to make the full width behave more like a div than a table, because the table will always try to contain its contents.

Here's a GIF:

responsive table

If we can accomplish a minimally responsive table in a different way, I'd love suggestions.

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Aug 30, 2018

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 30, 2018

Member

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.

Member

iseulde commented Aug 30, 2018

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.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

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.

Contributor

jasmussen commented Aug 30, 2018

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.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 30, 2018

Member

@jasmussen Cool, I'm fine with just restoring the display rule. Could we just add thead and tfoot with the tbody rule to display as a table as well? That makes sense right? Then it seems good to go.

Member

iseulde commented Aug 30, 2018

@jasmussen Cool, I'm fine with just restoring the display rule. Could we just add thead and tfoot with the tbody rule to display as a table as well? That makes sense right? Then it seems good to go.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

Will do, feel free to separately track any concerns you have and we'll look at it. 👍 👍

Contributor

jasmussen commented Aug 30, 2018

Will do, feel free to separately track any concerns you have and we'll look at it. 👍 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

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

Contributor

jasmussen commented Aug 30, 2018

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

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 30, 2018

Member

screen shot 2018-08-30 at 14 34 01

this is what I see when I create a new table

Member

gziolo commented Aug 30, 2018

screen shot 2018-08-30 at 14 34 01

this is what I see when I create a new table

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 30, 2018

Member

It seems like styles on the frontend are inverted:

Fixed width

screen shot 2018-08-30 at 14 38 11

Flexible width

screen shot 2018-08-30 at 14 38 46

Member

gziolo commented Aug 30, 2018

It seems like styles on the frontend are inverted:

Fixed width

screen shot 2018-08-30 at 14 38 11

Flexible width

screen shot 2018-08-30 at 14 38 46

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

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 block display.

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?

Contributor

jasmussen commented Aug 30, 2018

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 block display.

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?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

Oh, taking a look at the inverted styles in the Twenty theme.

Contributor

jasmussen commented Aug 30, 2018

Oh, taking a look at the inverted styles in the Twenty theme.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

Fix the fixed-width option in Twenty. Good catch.

screen shot 2018-08-30 at 15 37 40

screen shot 2018-08-30 at 15 37 44

screen shot 2018-08-30 at 15 37 51

screen shot 2018-08-30 at 15 39 04

Contributor

jasmussen commented Aug 30, 2018

Fix the fixed-width option in Twenty. Good catch.

screen shot 2018-08-30 at 15 37 40

screen shot 2018-08-30 at 15 37 44

screen shot 2018-08-30 at 15 37 51

screen shot 2018-08-30 at 15 39 04

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

Thanks, let's get this in and look at any other enhancements separately.

Contributor

jasmussen commented Aug 30, 2018

Thanks, let's get this in and look at any other enhancements separately.

@jasmussen jasmussen merged commit 07eeecf into master Aug 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/table-fix branch Aug 30, 2018

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 17, 2018

Contributor

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

It looks like this is the cause of #9779:
https://stackoverflow.com/a/12153220

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 display: table for table-layout: fixed; to work.

Unfortunately it looks like we need another approach. Probably a wrapping div as you say @jasmussen.

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.

Contributor

talldan commented Sep 17, 2018

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

It looks like this is the cause of #9779:
https://stackoverflow.com/a/12153220

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 display: table for table-layout: fixed; to work.

Unfortunately it looks like we need another approach. Probably a wrapping div as you say @jasmussen.

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.

@talldan talldan referenced this pull request Sep 17, 2018

Open

Add table dimension controls #9801

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment