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 header and body column misalignment (responsiveness using word-break: break-all) #10230

Merged
merged 2 commits into from Oct 3, 2018

Conversation

Projects
None yet
4 participants
@talldan
Contributor

talldan commented Sep 28, 2018

Description

Fixes #9779 - table header misalignment issue. This PR takes a different approach to that originally proposed in #10011, opting to wrap text instead of using a container div to handle an overflowing table.

Changes:

  1. The issue was caused by thead, tbody and tfoot elements being given the display: table property (SO explanation - https://stackoverflow.com/a/12153220). This styling has now been removed so that the elements have their default display property.
  2. table-layout: fixed; was previously applied on the thead, tbody and tfoot elements, but since they're no longer display: table that was no longer working. This style is now applied to the table element and the table element has been changed from display: block to display: table
  3. The above changes break the overflow-x: auto style that was applied to the table element. One option to solve this would have been to apply a wrapping div around the table. This was attempted on #10011, but ultimately the introduction of a wrapping element was decided against. This PR tries an alternative approach using word-break: break-all to ensure content in a table cannot overflow.

How has this been tested?

Column alignment

  1. Paste a table with a thead directly into the editor - e.g:
<table><thead><tr><td><strong>Column One</strong></td><td><strong>Column Two</strong></td></tr></thead><tbody><tr><td>one</td><td>two</td></tr><tr><td>three</td><td>four</td></tr></tbody></table>
  1. Observed in the editor that the table header columns are aligned with the body regardless of whether the 'Fixed width table cells' option is enabled.
  2. Observed in the preview that everything is aligned.

Overflow

  1. Enter long content without spaces or line breaks in a cell.
  2. Observed that the text wraps onto a new line and columns are sized according to the amount of content in a column.
  3. Observed that the the same behaviour is also present in the preview. (To test this, add_theme_support( 'wp-block-styles' ); needs to be added somewhere (e.g. in client-assets.php)).

Fixed width table cells

  1. Toggle fixed width table cells
  2. Ensured that table cells are equal spaced and that long content wraps in both the editor and preview

Screenshots

Before
screen shot 2018-09-18 at 6 09 04 pm

After
screen shot 2018-09-18 at 5 57 30 pm

Wrapping content
screen shot 2018-09-28 at 12 17 14 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

talldan added some commits Sep 13, 2018

Remove invalid `display` properties on table, thead, tbody, and tfoot
- `table` element needs to be `display: table` so that `table-layout: fixed`
works correctly.
- `thead`, `tbody` and `tfoot` need to use their default display properties,
otherwise columns become misaligned between header, body and footer. (#9779)
- Remove unecessary width on `thead`, `tbody`, and `tfoot` which is implicitly
100%

@talldan talldan self-assigned this Sep 28, 2018

@talldan talldan requested review from jasmussen and noisysocks Sep 28, 2018

@talldan talldan referenced this pull request Sep 28, 2018

Closed

Fix table header and body column misalignment #10011

4 of 4 tasks complete
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

Thanks so very much for working on this. Thank for pushing through despite all the feedback that keeps coming in, I know it's hard to keep the shipping spirit intact. This is great.

Fixed works 👍
Responsiveness with break-all is 🆗

So I say ship it. It's definitely clear that the horizontal scrollbar solution didn't work in the previous implementation — I'll still explore alternatives that don't require a wrapping div. But I also understand the desire to not have that wrapping div, which is totally fair.

I think it's an opportunity for a 3rd party block to create a new responsive table block, one that perhaps can reflow too, because it doesn't have to rely on the stock table markup like the core one does.

I still think the horizontal scrollbar is the better responsive solution, I would rather horizontally scroll a budget spreadsheet on my phone that have it reflow into single letter columns. But I recognize this is already a tall order on a phone.

👍 👍 from me, if not as a final solution, then at least an interim one until I can come up with a better solution.

Contributor

jasmussen commented Sep 28, 2018

Thanks so very much for working on this. Thank for pushing through despite all the feedback that keeps coming in, I know it's hard to keep the shipping spirit intact. This is great.

Fixed works 👍
Responsiveness with break-all is 🆗

So I say ship it. It's definitely clear that the horizontal scrollbar solution didn't work in the previous implementation — I'll still explore alternatives that don't require a wrapping div. But I also understand the desire to not have that wrapping div, which is totally fair.

I think it's an opportunity for a 3rd party block to create a new responsive table block, one that perhaps can reflow too, because it doesn't have to rely on the stock table markup like the core one does.

I still think the horizontal scrollbar is the better responsive solution, I would rather horizontally scroll a budget spreadsheet on my phone that have it reflow into single letter columns. But I recognize this is already a tall order on a phone.

👍 👍 from me, if not as a final solution, then at least an interim one until I can come up with a better solution.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

Here's an exploration that seems to work for responsiveness:

https://codepen.io/joen/pen/qJBYvz?editors=1100

The code is much simpler, and adds — as far as I can tell — a horizontal scrollbar on the table itself when necessary. It relies on setting the table itself to be block — would this be ok? CC: @mtias @iseulde

Contributor

jasmussen commented Sep 28, 2018

Here's an exploration that seems to work for responsiveness:

https://codepen.io/joen/pen/qJBYvz?editors=1100

The code is much simpler, and adds — as far as I can tell — a horizontal scrollbar on the table itself when necessary. It relies on setting the table itself to be block — would this be ok? CC: @mtias @iseulde

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 28, 2018

Contributor

@jasmussen - I did explore the approach in your codepen link, but unfortunately table-layout: fixed; only works if the table block is display: table;.

I think wrapping text is the pragmatic solution for now.

One way to support overflow could be to make it an option in the block sidebar, so that at least the user opts-in to it. It might be worth exploring that on a separate issue.

Contributor

talldan commented Sep 28, 2018

@jasmussen - I did explore the approach in your codepen link, but unfortunately table-layout: fixed; only works if the table block is display: table;.

I think wrapping text is the pragmatic solution for now.

One way to support overflow could be to make it an option in the block sidebar, so that at least the user opts-in to it. It might be worth exploring that on a separate issue.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

No reason for separate issues at the moment. We tried our best but it is seemingly not feasible for now.

Contributor

jasmussen commented Sep 28, 2018

No reason for separate issues at the moment. We tried our best but it is seemingly not feasible for now.

@iseulde

iseulde approved these changes Oct 3, 2018

Looks good if approved by @jasmussen!

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 3, 2018

Contributor

Yep. I appreciate all the care that's been put into this. I think we shouldn't let perfect be the enemy of good. I very much like the idea of at least improving the responsiveness beyond word-wrap with an off-by-default option in the sidebar, but toally good to ship this.

Contributor

jasmussen commented Oct 3, 2018

Yep. I appreciate all the care that's been put into this. I think we shouldn't let perfect be the enemy of good. I very much like the idea of at least improving the responsiveness beyond word-wrap with an off-by-default option in the sidebar, but toally good to ship this.

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Oct 3, 2018

Contributor

Thanks for the reviews!

Contributor

talldan commented Oct 3, 2018

Thanks for the reviews!

@talldan talldan merged commit d6af165 into master Oct 3, 2018

2 checks passed

codecov/project 48.53% remains the same compared to 3a210ab
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talldan talldan deleted the fix/table-misalignment-word-break branch Oct 3, 2018

@mtias mtias added this to the 4.0 milestone Oct 9, 2018

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