-
Notifications
You must be signed in to change notification settings - Fork 826
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
[EuiComboBox] Update to dogfood EuiTextTruncate
#7028
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Marco! As always, I super appreciate your contributions to EUI ❤️
I know you mention in your PR that this is a first attempt, so I kept my comments high level for now without diving into the details. The biggest thing is I think I'd like to make the logic generic as opposed to specific to EuiComboBox. LMK if that makes sense to you!
package.json
Outdated
@@ -212,6 +212,7 @@ | |||
"html-webpack-plugin": "^5.5.0", | |||
"inquirer": "^9.1.4", | |||
"jest": "^24.1.0", | |||
"jest-canvas-mock": "^2.5.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature adopts the Canvas api to measure text, so I had to add the jest-canvas-mock library for unit tests.
(I think this could be also implemented without the canvas, but it was easier that way).
I would personally lean towards avoiding the canvas API for this unless there's a strong reason to do so. My reasoning against is that it avoids an extra dependency and also because it's not something the EUI team is familiar with, and thus would be more difficult for us to maintain than just vanilla JS.
If canvas is absolutely the way to go, I'd then strongly prefer to use Cypress to test the functionality over adding a Jest dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change the code to not use Canvas API. I think same can be achieved using a div
element as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly like to see this be a generic EuiComponent, e.g. <EuiTextTruncate>
over a one-off added to EuiComboBox specifically. It will make unit testing & QA easier as well as allowing for fewer hard coded constants (e.g. allowing consumers to customize the ellipses, the character at which the truncation starts/ends, so on and so forth).
This might also play in well into supporting multi-line truncation (#6312), although that's obviously a nice to have and isn't as important as supporting customizable single line truncation for this use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this sounds like a longer shot than what I've planned. Not sure I can do it right now 😅
Oh, totally forgot to mention - if you don't have time right now to make a bunch of changes to this PR or work on it in depth, no worries, totally get it. EUI can take it over, but it's likely this will be a bit of a lower priority for us or we might not be able to do so for a few weeks. Just wanted to level-set expectations! |
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
EuiTextTruncate
src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx
Show resolved
Hide resolved
if (!this.props.searchValue) { | ||
return ( | ||
<EuiTextTruncate | ||
truncation="end" | ||
width={this.optionWidth} | ||
onResize={this.setOptionWidth} | ||
{...truncationProps} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[perf] I'm debating also not rendering EuiTextTruncate
at all if not searching and truncationProps
isn't passed (i.e., just falling back to native text-overflow: ellipsis
CSS).
The only thing that would actually affect for end-users is that EuiTextTruncate
intelligently detects if it needs truncation and conditionally adds a title
tooltip if truncated, and does not add one if not. But maybe that's not important?
FWIW EuiComboBox does use virtualization and real-world performance hasn't been an issue that I can see 🤔
@markov00 I'm seeing comments from you in my email inbox but I'm not able to find them in GitHub - apologies for not being able to respond in thread. Also sincerest apologies for not tagging you in on #7116 to review there. I'm happy to continue updating the new
Good catch, I'll change that to a
Very fair, but I'm a little confused about this statement. That original logic/approach was what @dej611 originally used and is what Lens is currently using. Is there another approach that's possible to use here that you're proposing or would rather see us use? Off the top of my head, we could try to detect string lengths past a certain amount (50? 100?) and start using exponential math (using half the remaining string length) instead of +/- 1 characters to reduce the amount of iterations. (there's an algorithm name for that that's escaping me right now, apologies for my lack of formal CS background). Would that help in terms of performance? |
Me too. I believed he deleted them after searching for 15 minutes in the thread...
I think the main problem there is that the DOM approach is way slower than the Canvas one, probably by a factor of 10x at least, which leads to worst cases for long texts. While with the Canvas approach the naïve approach of measuring one char at the time is sustainable, with DOM it can get really bad. But this is a general topic which I think apply less to this specific PR, with ComboBox, but might make more sense if Even if the logic here is optimized, i.e. with something like a binary search approach, the DOM approach is still slow and there are scenarios where it can get bad.
Exactly. It would be great to have some numbers here to back up the decision: something like the current approach with DOM vs Canvas AND the binary search approach with DOM vs Canvas. I guess only then we could get a more informative decision. |
For what it's worth, from an end-user-experience I'm seeing both Canvas and DOM approaches to both be fairly choppy when operating over a hundred lines and when being resized dynamically: https://eui.elastic.co/#/utilities/text-truncation#performance Agreed that DOM was worse than canvas, but IMO, virtualization made the biggest difference in terms of visible performance. Totally agreed that solid numbers will help (I'm a total noob at perf benchmarking and hoping to lean on you two for help with that). In the meanwhile, here's what I can do from the EUI side of things:
I would like the final outcome of our research to be a decision on the following:
|
Quick update to this: adding binary logic has made a MASSIVE difference in performance - 100 lines of truncated text in both canvas and DOM no longer need virtualization at all and is only minorly choppy when resized as opposed to majorly choppy. # of iterations from a 400 character string went down from 400+ to 12 :) I'm still figuring out the final algorithm for |
Hey all - we've received a high priority request from the Shared UX team to get in truncation options for The good news is that I have a new ratio-based logic that vastly improves performance and # of iterations (that I've seen, it maxes out at 10 iterations period regardless of length of text or available width). If no objections, I'll add you both @dej611 and @markov00 as reviewers for the new PR. |
Preview staging links for this PR:
|
💚 Build Succeeded
History
cc @cee-chen |
`v88.3.0`⏩`v88.5.0` closes #151514 --- ## [`88.5.0`](https://github.com/elastic/eui/tree/v88.5.0) - Updated `EuiCallOut` with a new `onDismiss` prop ([#7156](elastic/eui#7156)) - Added a new `renderCustomToolbar` prop to `EuiDataGrid`, which allows custom rendering of the toolbar. ([#7190](elastic/eui#7190)) - Added a new `allowResetButton` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows hiding the "Reset to default" button from the display settings popover. ([#7190](elastic/eui#7190)) - Added a new `additionalDisplaySettings` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows rendering extra settings inside the display settings popover. ([#7190](elastic/eui#7190)) - Updated `EuiDataGrid`'s toolbar display settings button icon ([#7190](elastic/eui#7190)) - Updated `EuiTextTruncate` with significantly improved iteration performance. Removed `measurementRenderAPI` prop, as `EuiTextTruncation` now only uses more performant canvas render API ([#7210](elastic/eui#7210)) - Updated `EuiPopover` with a new configurable `repositionToCrossAxis` prop ([#7211](elastic/eui#7211)) - Updated `EuiDatePicker` to support `compressed` input styling ([#7218](elastic/eui#7218)) - Added `gradient` and `palette` icon glyphs. ([#7220](elastic/eui#7220)) **Bug fixes** - Fixed `EuiPopover`'s missing animations on popover close ([#7211](elastic/eui#7211)) - Fixed `EuiInputPopover` anchoring to the wrong side and missing shadows on smaller screens ([#7211](elastic/eui#7211)) - Fixed `EuiSuperDatePicker` icon spacing on the quick select button ([#7217](elastic/eui#7217)) - Fixed a missing type in `EuiMarkdownEditor`'s default processing plugins ([#7221](elastic/eui#7221)) ## [`88.4.1`](https://github.com/elastic/eui/tree/v88.4.1) **Bug fixes** - Fixed missing `className`s on `EuiTextTruncate` ([#7212](elastic/eui#7212)) - Fixed `title`s on `EuiComboBox` dropdown options to always be present ([#7212](elastic/eui#7212)) - Fixed `EuiComboBox` truncation issues when search is an empty space ([#7212](elastic/eui#7212)) ## [`88.4.0`](https://github.com/elastic/eui/tree/v88.4.0) - Updated `EuiComboBox` to allow configuring text truncation behavior via `truncationProps`. These props can be set on the entire combobox as well as on on individual dropdown options. ([#7028](elastic/eui#7028)) - Updated `EuiInMemoryTable` with a new `searchFormat` prop (defaults to `eql`). When setting this prop to `text`, the built-in search bar will ignore EQL syntax and allow searching for plain strings with special characters and symbols. ([#7175](elastic/eui#7175)) **Bug fixes** - `EuiComboBox` now always shows the highlighted search text, even on truncated text ([#7028](elastic/eui#7028)) - Fixed missing i18n in `EuiSearchBar`'s default placeholder and aria-label text ([#7175](elastic/eui#7175)) - Fixed the inline compressed styles of `EuiDescriptionListTitle` to use a taller line-height for readability ([#7185](elastic/eui#7185)) - Fixed `EuiComboBox` to correctly truncate selected items when displayed as pills and plain text ([#7193](elastic/eui#7193)) **Accessibility** - Added `aria-current` attribute to `EuiTablePagination` ([#7186](elastic/eui#7186)) **CSS-in-JS conversions** - Converted `EuiDroppable` and `EuiDraggable` to Emotion; Removed `$euiDragAndDropSpacing` Sass variables ([#7187](elastic/eui#7187)) --------- Co-authored-by: Patryk Kopycinski <contact@patrykkopycinski.com> Co-authored-by: Jan Monschke <jan.monschke@elastic.co> Co-authored-by: Thomas Watson <watson@elastic.co>
Summary
This PR updates
EuiComboBox
to dogfoodEuiTextTruncate
, which is a new component that spawned directly from Marco's awesome original work in this PR (#7116). This allows consumers to fully customize the truncation styles of dropdown options with long text.Previous PR description from Marco
This PR is a first attempt to add a `middle` truncation feature to the `ComboBox` component as in #5997 . The approach is based on a similar solution we adopted in Lens with a custom implementation. Here I took advantage of the measures already taken for the combobox component so I managed to get rid of some custom code.The feature adopts theCanvas
api to measure text, so I had to add thejest-canvas-mock
library for unit tests.(I think this could be also implemented without the canvas, but it was easier that way).
The
![Screenshot 2023-08-02 at 14 49 47](https://private-user-images.githubusercontent.com/924948/257857758-e67926db-ccbb-4c24-8d4b-a4eb7aaadf03.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjM5NzU2OTEsIm5iZiI6MTcyMzk3NTM5MSwicGF0aCI6Ii85MjQ5NDgvMjU3ODU3NzU4LWU2NzkyNmRiLWNjYmItNGMyNC04ZDRiLWE0ZWI3YWFhZGYwMy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwODE4JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDgxOFQxMDAzMTFaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0yMzJmYzNkOGMyNmMzODQ1MTc1MGMzYzIzMTgyZTFlNjA1MTNlMzI4NDhhZWIxMjZhNWIwMDg4NTlhZjE2N2FjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.JeSAXLIIqbvDYyyVAFG8xVGyM6C3Nd56LADwnuqGPr8)
end
truncation logic is still leveraging CSS.for the calculation I had to use some hardcoded values, but maybe they could be computed from some theme source?
QA
General checklist
@default
if default values are missing)and playground togglesjest andcypress tests[ ] Updated the Figma library counterpart- Note - this probably needs to be done so designers know this is an option, but I'm not totally sure how to do so. I'll ask at the next EUI Figma meeting- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked Code Sandbox works for any docs examples