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 word-break in tables on most browsers #1349

Merged
merged 9 commits into from
Dec 7, 2018
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Dec 5, 2018

The problem came about that browser's were breaking on words unnecessarily (in Firefox and IE11). The problem is that neither Firefox nor IE support word-break: break-word and so there is a fallback to word-break: break-all to ensure that long words don't expand past container.

What it looked like before:

So this PR changes from using the word-break: break-word approach to using overflow-wrap: break-word which is supported by all browsers except for IE11 and behaves just like word-break.

This is what it looks like now:

Helpers

I've added both a mixin (euiTextOverflowWrap()) and CSS utility (.eui-textOverflowWrap) to handle the ease of using overflow-wrap and the IE fallback.

Caveat

There is one big caveat to using overflow-wrap. It does not work on items that have display: flex. Wah wah...

Where this becomes an issue is when passing textOnly: false to EuiTableRowCell which removes the containing .euiTableCellContent__text and places the children as a direct descendent of euiTableCellContent which, guess what?, is display: flex. 😢

So it has to use the word-break implementation, meaning it will also use the break-all fallback for Firefox.

This usually isn't an issue because the default prop is textOnly: true. However, EuiBasicTable will turn that prop to false if you use the render function.

What it looks like when textOnly: false:

Firefox will still break on all because the containing element is a flex element.

I've updated the documentation to try to spell out how to fix this, usually by keeping/adding textOnly: true. But another way to combat this is to just wrap the text node with the .eui-textOverflowWrap utility class.

IE11

Blech. Unfortunately, IE11 has no support any type of break-word so it will continue to use the break-all fall back.

screen shot 2018-12-06 at 14 48 20 pm

Checklist

  • [ ] This was checked in mobile
  • This was checked in IE11
  • [ ] This was checked in dark mode
  • Any props added (modified) have proper autodocs
  • Documentation examples were added (modified)
  • A changelog entry exists and is marked appropriately
  • [ ] This was checked for breaking changes and labeled appropriately
  • [ ] Jest tests were updated or added to match the most common scenarios
  • [ ] This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

@cchaos
Copy link
Contributor Author

cchaos commented Dec 5, 2018

@cjcenizal Would you mind taking a peak at this PR since you were the one who originally added the word-wrap stuff?

@peteharverson Can you check if this fixes your issue? IE11 will still behave poorly because it just doesn't support word-break.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

@cchaos Do you think we could update the examples to demonstrate the bug with the original styles, so we can then verify that the new styles fix it?

@cchaos
Copy link
Contributor Author

cchaos commented Dec 6, 2018

@cjcenizal & @peteharverson Based on more investigating, I've updated the description of the PR and changed the implementation of the "fix".

cc @snide

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Looks great! Nice analysis. Thanks for digging in and explaining everything so clearly. I just had a couple minor suggestions.

CHANGELOG.md Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor Author

cchaos commented Dec 7, 2018

@cjcenizal Is this documentation more helpful?

screen shot 2018-12-07 at 14 06 59 pm

@cchaos cchaos merged commit a5dbb39 into elastic:master Dec 7, 2018
@cchaos cchaos deleted the break-all branch December 7, 2018 20:26
@snide snide mentioned this pull request Dec 7, 2018
3 tasks
@peteharverson
Copy link

Confirm this fixes the wrapping of text in the column header of the ML jobs table (all browsers except IE).

Firefox before fix:
jobs_table_wrap_1

Firefox after fix:
jobs_table_wrap_2

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