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.

Sign up for GitHub

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

Jump to bottom

[EuiComboBox] Fixes/reversions for Kibana FTR #7212

Merged
merged 4 commits into from
Sep 21, 2023
Merged

[EuiComboBox] Fixes/reversions for Kibana FTR #7212

merged 4 commits into from
Sep 21, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 21, 2023 •
edited
Loading

Summary

It turns out Kibana's FTR test service is incredibly dependent on the title attribute to select combobox options by so me removing that in #7028 was A Huge Mistake 😭

While here, I've decided to address #7028 (review) after all by not instantiating the truncation component (and letting CSS text-overflow: ellipsis do its thing) if no custom truncationProps configuration(s) are passed.

Also EuiTextTruncate was missing classNames (yay, betas 💀) so I added those while here.

QA

  • Go to https://eui.elastic.co/pr_7212/#/forms/combo-box#truncation
  • Confirm the various truncation options still render correctly/as expected
  • Go to the first example on the page, and confirm the long option (Pandora) still truncates at the end via CSS
  • Confirm all options, including options without truncation, have title tooltips

General checklist

  • Browser QA
    • Checked in Chrome, Safari, Edge, and Firefox
  • Docs site QA - N/A
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

@cee-chen
[setup] Add missing EuiTextTruncate classes
82f0792
@cee-chen
Fix Kibana FTR shenanigans 😭
78f904f 
- turns out removing title was a terrible mistake

- also selenium gets big mad about the overlaid full text, might as well mitigate it now
@cee-chen
[optional] Misc combobox cleanups
18d7946 
- [perf] remove truncation component completely if truncation isn't customized; fall back to CSS overflow instead

- fix buggy behavior where truncation would suddenly change if search value only consists of spaces (by trimming condition early)
@cee-chen
changelog
b0a5698
@cee-chen cee-chen marked this pull request as ready for review September 21, 2023 17:25
@cee-chen cee-chen requested a review from a team as a code owner September 21, 2023 17:25
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

breehall
breehall approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

Confirmed in the PR preview that:

  1. All options (with and without) truncation have a title
  2. Examples that don't have truncation props still truncate properly with CSS
  3. The various versions of truncation still work

No questions in regards to how or why we've added logic to use CSS for truncation vs. using the new truncation component. I also understand why we need the pointer-events: none for the truncation class and FTR tests. It feels like you found a needle in a haystack with that one.

@cee-chen
Copy link
Member Author

Thanks Bree!

It feels like you found a needle in a haystack with that one.

Haha, yesterday was more like stabbing myself with a needle repeatedly 😭 I kept trying to get text replacement working (ended up going down a fun xpath rabbit hole for that...) but honestly it's just not worth it, restoring title is so much simpler 🤦

@cee-chen cee-chen merged commit 26ff549 into elastic:main Sep 21, 2023
8 checks passed
@cee-chen cee-chen deleted the combobox-patch branch September 21, 2023 18:10
@cee-chen cee-chen mentioned this pull request Sep 21, 2023
Merged
jbudz pushed a commit to elastic/kibana that referenced this pull request Sep 27, 2023
@cee-chen @patrykkopycinski @janmonschke
Upgrade EUI to v88.5.0 (#166868)
581b30b 
`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 <[email protected]>
Co-authored-by: Jan Monschke <[email protected]>
Co-authored-by: Thomas Watson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breehall breehall breehall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cee-chen @kibanamachine @elasticmachine @breehall