Add EuiTextTruncate component
#7116
Conversation
TODO: tests TODO: figure out how to incorporate combobox's search highlighting TODO: update combobox to dogfood this component
- Write prop docs - Rename `separator` prop to `ellipsis` - Tweak ellipsis check to return an empty string / not completely throw and simply error instead - Resize observer - separate to its own component for performance/readability, + tweak conditional to check for `undefined` rather falling back if `0` (may matter for initial render) - Rename internal `EuiTextTruncateToWidth` to `EuiTextTruncateWithWidth`
since we're already defaulting to `end`
- clean up storybook props DX some - clean up useMemo logic for `truncation` and `truncationOffset` props - prefer moving all logic to a single memo that returns multiple values
- majority of testing is in Cypress, since JSdom has no concept of dimensions/width
+ fix resizable story to use inline resize on supported browsers
- at the cost of more repetitive DOM, but I think the tradeoff here is worth it
- the new utils allowed me to see how to attempt to continue truncating the logic rather than just metaphorically throwing hands in the air!
- `truncationPosition` can't be set back to undefined if it starts as a number
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7116/ |
…ethods + write more detailed jsdoc comments/descriptions + add a Cypress spec to confirm canvas util works correctly
- param should be optional
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7116/ |
- apparently has zero fonts in common with my machine, so google fonts it is + clean up previous font loading code - we only need 1 weight, and we can use `before()` instead of a mount util
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7116/ |
+ harden standalone behavior
+ re-add the font computation method Marco originally had in the canvas class, and require passing in either a font string or a container to compute fonts from
+ example where the perf differences between various recommended mitigations can be tested
- add basic unit test for confirming that the right utils areb eing called - add missing E2E test for resize observer behavior
- remove truncation error testing from the main component - we're already testing that in the utils unit tests - reorganize utils E2E tests slightly for readability - misc wording fixes
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7116/ |
|
Hey y'all! I know the line diff on this is probably pretty intimidating, but I promise about half of that is just docs and tests 🤞 Feel free to split up your review by batches if it it helps - e.g. review the tests, then the actual source code, then documentation. @tkajtoch Was hoping you'd be able to pick up code review on this, but if you don't have time, no worries. I feel some amount of confidence in the code thanks to Marco having had eyes on this, although of course I'd appreciate yours as well. @1Copenut, I'm particularly hoping for your help QAing the screen reader experience on JAWS and NVDA. I initially had something like this in terms of DOM markup: <div aria-label="Full text">...truncated text...</div>Unfortunately, VO+Safari would constantly read out "empty group" when navigating to the text, so I ended up rendering the truncated and full text separately so that VO would correctly identify "You are on a text element". I'd be curious to hear if JAWS/NVDA have any similar issues or if they can parse this component correctly. edit to add: VO+Firefox still has the |
There was a problem hiding this comment.
[attaching to a semi-random file for threading]
Just want to note... I want to acknowledge I probably wrote too many tests for this component 🤦 What happened was this:
EuiTextTruncatewas the only file with literally everything in it at first, and I wrote skeleton unit tests for it (since jsdom can't make it past the!widthcheck) and really comprehensive E2E tests for it- Marco made the excellent suggestion of pulling out the truncation logic to a separate utility function, which spawned
TruncationUtilsand suddenly made it way easier to mock widths/jsdom and unit test functionality, so I ended up writing comprehensive unit tests and sorta-skeleton E2E tests forutils.tsas well
I probably have too many tests for EuiTextTruncate at this point that are already handled by util tests 😅 I tried to clean some up but in the end I figured the extra confidence couldn't hurt.
LMK if you think it's an issue / if tests are either super laborious to read or will be a pain to maintain in the future - I can certainly look into pruning/de-duplicating more code!
There was a problem hiding this comment.
👍 I took this for a drive with the big three screen readers. All three read out the full text and didn't stumble on the ellipses. The extra element that allows me to copy/paste a full line is going to pay huge dividends.
Great component and PR @cee-chen !
|
Thanks for the screen reader QA Trevor, very excited to hear it's working well on all 3 SRs! Just curious, do you have any feedback on the new docs page? Do you feel like all the examples and copy were pretty straightforward to follow, or were there any confusion points? |
@cee-chen I thought the examples were clear and concise. I had a good understanding of props and use cases when I was done testing. |
| /** | ||
| * By default, EuiTextTruncate will render the truncated string directly. | ||
| * You can optionally pass a render prop function to the component, which | ||
| * allows for more flexible text rendering, e.g. adding custom markup | ||
| * or highlighting | ||
| */ | ||
| children?: (truncatedString: string) => ReactNode; |
|
|
||
| /* istanbul ignore next */ | ||
| get textWidth(): number { | ||
| throw new Error('This function must be superseded by a DOM or Canvas util'); |
There was a problem hiding this comment.
I'd love to see _TruncationUtils being an abstract class and have textWidth and setTextToCheck defined as abstract as well to get full type checking on these.
There was a problem hiding this comment.
Oooo, this is where my lack of CS background comes through and I confess I'm actually a total noob at classes. Let me look up abstract classes and look into refactoring this!
There was a problem hiding this comment.
This is the closest I've ever come to feeling like an IRL wizard. Thanks for teaching me this Tomasz!!! This is such a lovely and clean refactor! 🤩 7ec0ee9
Linking to the docs I used in case anyone else comes across this thread and is interested in learning: https://www.typescriptlang.org/docs/handbook/2/classes.html#abstract-classes-and-members
I was also super curious how Typescript would handle the concept of an abstract class in final compiled JS (as the stackoverflow answers I found pre-Typescript recommended throwing an error in the constructor), so I did a yarn build and inspected the file in lib/. It looks like it still exists as its own var, but we should be fairly safe still as I opted not to export _TruncationUtils to prevent it from being used standalone by consumers.
| fullText: SharedParams['fullText']; | ||
| ellipsis: SharedParams['ellipsis']; | ||
| availableWidth: SharedParams['availableWidth']; |
There was a problem hiding this comment.
We should make these protected if we decide to go with the abstract class implementation
There was a problem hiding this comment.
Thanks for this specification!! 7ec0ee9
It makes total sense to do, and I wouldn't have thought of it myself (I'm very lazy when it comes to the concept of private/public vars haha - probably because of my filthy jQuery roots where everything is a global and nothing matters 🤣)
There was a problem hiding this comment.
I LOVE this! I'm glad you decided to implement the Canvas 2D Context version too 🎉 As far as I can tell the measureText API is available for all browsers we support. Is there a specific reason why we're still including the 'dom' option and why is it our default?
Super great question Tomasz! Here's my reasoning, although I'm 100% open to being argued against it. When I was originally writing this code, I was doing so with the mindset of trying to get it as close to native As Marco V mentioned in #7028 (comment), there are differences in the Canvas rendering engine (which you can notice with text being off by 1-2 characters once width gets long enough). I found some additional ones while testing the new canvas implementation - the The way I see it, the DOM approach is the most accurate to what users see on the page, and accounts for all possible font CSS consumers may have applied to the text or to their app in general. Which admittedly is probably going to be fairly edge-case - so if you think performance trumps accuracy by default in this case, I'd be totally fine with that. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7116/ |
💚 Build Succeeded
History
|
Yeah, I think 2D Canvas still doesn't do subpixel antialiasing on text and it doesn't work well with subpixel font sizes that we usually use, partially because of defining sizes in |
|
Thanks y'all, super excited for this! |
Major changes in this PR:
## Removal of `.euiAccordionForm` classNames
EUI is moving away from providing global `classNames` styles for
components - where possible, we want to provide props as opposed to
styles. As part of our ongoing Emotion conversion, we have removed the
following `EuiAccordion`-specific classes:
- `.euiAccordionForm` (replaced with `borders="horizontal"`)
- `.euiAccordionForm__button` (replaced with `buttonProps={{
paddingSize: 'm' }}`)
- `.euiAccordionForm__title` styles - this was only removing text
underlines on hover. If still desired, re-add this behavior with custom
CSS.
- `.euiAccordionForm__extraAction` - there was 1 usage of this in Kibana
in Watcher, which was converted to one-off custom inline Emotion CSS
instead.
This change accounts for the first 3-4 commits in the PR. ⚠️ If your
team was one of the 4-5 teams affected by this change, we would greatly
appreciate your help QAing the UI and ensuring it looks as desired/the
same as before.
## Fixed `EuiHeader` affordance
The Sass `euiHeaderAffordForFixed` mixin has been deprecated and
replaced by a global `--euiFixedHeadersOffset` CSS variable. This
variable updates independently and dynamically based on the number of
fixed headers on the page, and is usable by both Sass and Emotion. All
underlying EUI components that need to account for fixed headers (such
as flyouts and page sidebars/templates) have been updated to consume
this new variable.
This change cleans up a great deal of Sass code, and is incidentally
extremely timely with serverless efforts (as serverless has only one
fixed header and the classic Kibana chrome has two).
This change constitutes a majority of the commits in this PR, which
involve removing various fixed Sass header variables and replacing them
with the new CSS variable. I strongly recommend [reviewing changes by
commit if
possible](https://github.com/elastic/kibana/pull/165790/commits) to see
any associated changes that make sense together with any of your touched
file(s). ⚠️ If your team was affected by this change (primarily due to
custom header layouts), your help would be greatly appreciated QAing
your app to ensure no UI regressions in that regard have occurred.
---
`v88.1.0`⏩ `v88.2.0`
## [`88.2.0`](https://github.com/elastic/eui/tree/v88.2.0)
- Added a new `EuiTextTruncate` component, which provides custom
truncation options beyond native CSS
([#7116](elastic/eui#7116))
- Fixed-positioned `EuiHeader`s now set a global CSS
`--euiFixedHeadersOffset` variable, which updates dynamically based on
the number of fixed headers on the page.
([#7144](elastic/eui#7144))
- `EuiFlyout`s now dynamically set their position, height, and mask
based on the number of fixed headers on the page.
([#7144](elastic/eui#7144))
- Sticky-positioned `EuiPageSidebar`s now dynamically set their position
and height based on the number of fixed headers on the page. This can
still be overridden via the `sticky.offset` prop if needed.
([#7144](elastic/eui#7144))
- `EuiPageTemplate` now dynamically offsets content from any fixed
headers on the page. This can still be overridden via the `offset` prop
if needed. ([#7144](elastic/eui#7144))
- Updated `EuiAccordion` with a new `borders` prop
([#7154](elastic/eui#7154))
- Updated `EuiAccordion` with a new `buttonProps.paddingSize` prop
([#7154](elastic/eui#7154))
**Deprecations**
- Deprecated the Sass `euiHeaderAffordForFixed` mixin. Use the new
global CSS `var(--euiFixedHeadersOffset)` variable instead.
([#7144](elastic/eui#7144))
**CSS-in-JS conversions**
- Except for generic CSS utilities, EUI is moving away from providing
global `classNames` that are component-specific. As part of this effort,
we have removed the following `EuiAccordion`-specific classes:
([#7154](elastic/eui#7154))
- Removed `.euiAccordionForm` styles. Use the `borders="horizontal"`
prop instead
- Removed `.euiAccordionForm__button` styles. Use the `buttonProps={{
paddingSize: 'm' }}` prop instead
- Removed `.euiAccordionForm__extraAction` styles. Convert this to your
own custom CSS if necessary.
- Removed `.euiAccordionForm__title` styles. Convert this to your own
custom CSS if necessary.
---------
Co-authored-by: Stratoula Kalafateli <[email protected]>
Summary
This standalone component was created from @dej611's proposal for customizable truncation for
EuiComboBox. Once this PR merges, we'll update #7028 to dogfood the new component.Screencaps
Note that highlighting truncated text will see a significant UX improvement with this utility compared to previously (highlighted text that was out of view would not be visible to users).
QA
General checklist
@defaultif default values are missing)and playground toggles- skipping playground due to imminent move to Storybook- [ ] Checked in both light and dark modes- N/A, doesn't apply to this component- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Updated the Figma library counterpart- I don't think this needs a Figma counterpart since it's purely a utility