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 colspan bug in table block for tables with <thead> tags #7899

Merged
merged 1 commit into from
Aug 7, 2018

Conversation

earnjam
Copy link
Contributor

@earnjam earnjam commented Jul 11, 2018

Fixes: #7688

Removing display: table from the tbody tag in the table block appears to solve the issue. I didn't see any negative effects from this in my testing, but it could use some additional testing to make sure there isn't a scenario I didn't come across.

Tagging @jasmussen since he added this line originally in 24011e3 in case it's required for something.

Checklist:

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

@earnjam earnjam requested a review from jasmussen July 11, 2018 17:31
@jasmussen
Copy link
Contributor

I'm afk for a bit so you might want to expand the review range if you'd like a prompt review, but as a sanity check and without testing, this seems good. However you need to be sure to test both the options for fixed width cells in the inspector, and verify that the responsiveness works, that is there is a horizontal scroll bar if there's content on mobile that can't wrap.

@earnjam
Copy link
Contributor Author

earnjam commented Jul 11, 2018

Thanks, no rush. I'm just working through some of the block conversion issues to try to whittle those down ahead of Try Gutenberg. I'll test out some more fixed-width cell scenarios to make sure they work.

@jasmussen jasmussen requested a review from a team July 12, 2018 06:27
Copy link
Contributor

@ianbelanger79 ianbelanger79 left a comment

Choose a reason for hiding this comment

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

Looks good to me, removing display: table; from .wp-block-table tbody does fix the table display in the editor

@earnjam earnjam merged commit 248fc29 into master Aug 7, 2018
@earnjam earnjam deleted the fix/7688-thead-colspan branch August 7, 2018 19:39
@earnjam earnjam added this to the 3.5 milestone Aug 7, 2018
@jasmussen
Copy link
Contributor

Sorry for belatedly returning to this PR. I think it broke this control:

screen shot 2018-08-09 at 13 53 54

How it should work:

fixed widths

How it's broken now:

broken

@earnjam
Copy link
Contributor Author

earnjam commented Aug 9, 2018

Ah, good catch. Sorry about that. Looks like instead of removing display: table, instead we need to just apply the same rules to thead that are on tbody

@earnjam
Copy link
Contributor Author

earnjam commented Aug 9, 2018

@jasmussen Should I open a new PR with the fix or consolidate to #8767?

@jasmussen
Copy link
Contributor

I don't think a fix is urgent. If we know how to fix it, so long as it's on someone's radar or tracked, either is fine. Thanks

@jasmussen
Copy link
Contributor

If you have a recommended fix, I can put together a PR.

@jasmussen
Copy link
Contributor

@earnjam It's important we do not ship 3.7 without this fix. Given that the fixed table cells were not part of #8767, would you like to create a PR for this?

Otherwise let me know how you'd propose the fix, and I can put together.

Alternatively we'll have to revert this.

jasmussen pushed a commit that referenced this pull request 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.
jasmussen added a commit that referenced this pull request Aug 30, 2018
* 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.

* Address feedback: include thead, tfoot

* Fix collapsed on init table.

* Fix fixed-width cells.
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.

3 participants