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

Table block: Removes the word-break:break-all; from the table cells #16741

Merged
merged 4 commits into from
Aug 6, 2019

Conversation

mapk
Copy link
Contributor

@mapk mapk commented Jul 24, 2019

Fixes #16740. Removes the word-break:break-all; from the CSS for the Table cells.

How has this been tested?

Locally.

Screenshots

BEFORE:

Screen Shot 2019-07-24 at 10 06 56 AM

AFTER:

table-breaks

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@mapk mapk added Needs Testing Needs further testing to be confirmed. [Block] Table Affects the Table Block labels Jul 24, 2019
@mapk mapk self-assigned this Jul 24, 2019

td,
th {
// Aligned tables shouldn't scroll horizontally so we need their contents to wrap.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I did some alignment testing as can be seen in the gif, I'm a little curious about this comment. Do the contents need to wrap by break-all? Or can they just wrap naturally? I'm hoping my removal of this doesn't resurface an issue I'm unaware of. @jasmussen @talldan any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this to overflow-wrap seems to be the better solution.

Copy link
Contributor

@talldan talldan Jul 25, 2019

Choose a reason for hiding this comment

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

I think there's an issue with overflow-wrap. If you type in some text with no spaces along and use a block alignment, the table goes off the screen:
Screen Shot 2019-07-25 at 11 41 17 am

I think the key behaviour is that when an alignment is used the table should only ever take up a portion of the page, it should never expand to full width and beyond. word-break: break-word; might be the best compromise. That still seems to wrap long words, but prefers breaking at word boundaries where possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for that catch, @talldan! Using word-break: break-all was just so difficult for me b/c it broke a LOT of words, at least in Firefox. If it's not complicated, what if we used overflow-wrap for the default aligned Table block, and overwrite that with break-word for any Table block that uses alignments?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Yep, great idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mapk I think what you want to do here is update the current word-break: break-all to word-break: break-word instead of replacing with overflow-wrap. That should accomplish what you want without causing issues with long words not wrapping.

I see @talldan mentioned that above, but I know from working with this in the past it's easy to miss that distinction while reading things so thought I might point it out.

Copy link
Contributor

@brentswisher brentswisher left a comment

Choose a reason for hiding this comment

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

I did notice one thing while testing this to consider. If a language uses very long words, they will now be shot off the side of the screen, with no way for the user to get to the content. I don't know how often a word in normal usage (in any language) would actually be long enough this would be an issue, but thought it might be worth mentioning.

Current:
Gutenberg_Dev

This Branch:
dauSJ6G0u5

Tested using the german translation of "Orange juice concentrate tastes like two thousand one hundred twenty-four oranges." : Orangensaftkonzentrat schmeckt nach zweitausendeinhundertvierundzwanzig Orangen. because I knew it would have long words and my German is extremely limited 😄 🇩🇪

…ly supported and works better than word-wrap:break-all.
@mapk
Copy link
Contributor Author

mapk commented Jul 25, 2019

Thanks for the test, @brentswisher! I've updated the CSS to use overflow-wrap:break-word instead of word-wrap:break-all because it tends to do better with shorter words, just dropping them to another line instead of breaking them needlessly, but it still breaks the longer words. I needs me some more testing.

Overflow-wrap
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-wrap
https://caniuse.com/#search=overflow-wrap

Editor:

Screen Shot 2019-07-24 at 5 06 42 PM

Frontend:

Screen Shot 2019-07-24 at 5 06 50 PM

@talldan
Copy link
Contributor

talldan commented Jul 25, 2019

Left a comment in the thread above - #16741 (comment)

@mapk
Copy link
Contributor Author

mapk commented Jul 31, 2019

Good catch, @brentswisher here: #16741 (comment). And sorry I missed that earlier suggestion, @talldan. I just need some testing now to confirm the fix.

Copy link
Contributor

@brentswisher brentswisher left a comment

Choose a reason for hiding this comment

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

Looks like you missed one line in your last commit swapping overflow-wrap with word-break, I commented on that line, it's still causing the table to flow out of the page:
skitch

packages/block-library/src/table/style.scss Outdated Show resolved Hide resolved
@talldan talldan added the [Type] Enhancement A suggestion for improvement. label Aug 6, 2019
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

This is a nice improvement, works great in my testing, so I'm going to merge it! Thanks @mapk. 🎉

Also props to @brentswisher for all the reviews!

@talldan talldan dismissed brentswisher’s stale review August 6, 2019 04:56

Changes now addressed.

@talldan talldan merged commit eba92cb into master Aug 6, 2019
@talldan talldan deleted the update/table-remove-word-breaks branch August 6, 2019 04:56
@youknowriad youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
…#16741)

* Fixes #16740. Removes the word-break:break-all; from the CSS for the Table cells.

* Added the overflow-wrap:break-word attribute which appears to be widely supported and works better than word-wrap:break-all.

* Changed to word-break:break-word as per @brentswisher's wonderful catch.

* Missed an overflow-wrap. Got it now.
gziolo pushed a commit that referenced this pull request Aug 29, 2019
…#16741)

* Fixes #16740. Removes the word-break:break-all; from the CSS for the Table cells.

* Added the overflow-wrap:break-word attribute which appears to be widely supported and works better than word-wrap:break-all.

* Changed to word-break:break-word as per @brentswisher's wonderful catch.

* Missed an overflow-wrap. Got it now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Table Affects the Table Block Needs Testing Needs further testing to be confirmed. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table block: Words in cells allow break-all and look really funky
4 participants