[EuiComboBox] Update to dogfood EuiInputPopover
#7246
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
+ rip out custom EuiPortal and EuiPopoverPanel usage - there's still a lot more logic to rip out which EuiInputPopover already handles, this is just the minimum needed to maintain working behavior
- underlying popover component takes care of that + remove `euiBody-hasPortalContent` logic (if underlying EuiPopover/EuiPortal components don't need it, combobox shouldn't either) - note: type exported from index was not being used by any consumer-facing component, so I don't think it's a breaking change to remove it - TODO: pass resize-observer `width` from EuiInputPopover to combobox options component
- EuiPopover already handles this automatically under the hood + add regression test for this, since this had a issue filed in Kibana at some point (3551)
- TODO: should `EuiSuperSelect` use this new prop as well?
- EuiInputPopover is already managing this via resize observer
- by passing more elegant props to virtualized `FixedSizeList` component
- no idea if this breaks production because there are apparently no tests for this, but it looks unnecessary - we should be ready to roll this individual commit back if necessary / if Kibana breaks
- friendship with enzyme is over, RTL is my new best friend - remove snapshots that weren't asserting anything meaningful - turn off whitespace changes, etc etc - I probably should have broken this up a bit more into separate PRs but tbh these tests aren't great and will almost certainly need to be rewritten from scratch once we convert EuiComboBox to EuiSelectable in any case, so f it
cee-chen
force-pushed
the
combobox-input-popover
branch
from
9532b07
to
3cee693
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Despite this not being a "breaking" change, this may be worth doing a pre-release for and testing against Kibana CI (particularly FTR and Cypress). Unfortunately, Kibana uses a lot of comboboxes in many places and combobox E2E are somewhat... fragile. Better to catch stuff now than have to do a bunch of patches later. |
|
Preview staging links for this PR:
|
1Copenut
approved these changes
Oct 3, 2023
|
Prerelease CI test: elastic/kibana#168053 No FTR failures (🤯 🎉) and Jest tests are snapshots/fairly quick fixes. Hooray! Going ahead and merging this for next week's release. |
This was referenced Oct 5, 2023
1Copenut
pushed a commit
to elastic/kibana
that referenced
this pull request
Oct 11, 2023
`v88.5.4`⏩`v89.0.0` --- ## [`89.0.0`](https://github.com/elastic/eui/tree/v89.0.0) - Added new `pushAnimation` prop to push `EuiFlyout`s, which enables a slide in animation ([#7239](elastic/eui#7239)) - Updated `EuiComboBox` to use `EuiInputPopover` under the hood ([#7246](elastic/eui#7246)) - Added `inputPopoverProps` to `EuiComboBox`, which allows customizing the underlying popover ([#7246](elastic/eui#7246)) - Added a new beta `EuiTextBlockTruncate` component for multi-line truncation ([#7250](elastic/eui#7250)) - Updated `EuiBasicTable` and `EuiInMemoryTable` to support multi-line truncation. This can be set via `truncateText.lines` in the `columns` prop. ([#7254](elastic/eui#7254)) **Bug fixes** - Fixed `EuiFlexGroup` and `EuiFlexGrid`'s `m` gutter size ([#7251](elastic/eui#7251)) - Fixed focus trap rerender issues in `EuiFlyout` with memoization ([#7259](elastic/eui#7259)) - Fixed a visual bug with `EuiContextMenu`'s animation between panels ([#7268](elastic/eui#7268)) **Breaking changes** - EUI's global body font-size now respects the `font.defaultUnits` token. This means that the global font size will use the `rem` unit by default, instead of `px`. ([#7182](elastic/eui#7182)) - Removed exported `accessibleClickKeys`, `comboBoxKeys`, and `cascadingMenuKeys` services. Use the generic `keys` service instead ([#7256](elastic/eui#7256)) - Removed `EuiColorStops` due to low usage ([#7262](elastic/eui#7262)) - Removed `EuiSuggest`. We recommend using `EuiSelectable` or `EuiComboBox` instead ([#7263](elastic/eui#7263)) - Removed `euiHeaderAffordForFixed` Sass mixin, and `$euiHeaderHeight` and `$euiHeaderHeightCompensation` Sass variables. Use the CSS variable `--var(euiFixedHeadersOffset, 0)` instead. ([#7264](elastic/eui#7264)) **Accessibility** - When using `rem` or `em` font units, EUI now respects, instead of ignoring, browser default font sizes set by end users. ([#7182](elastic/eui#7182))
dej611
pushed a commit
to dej611/kibana
that referenced
this pull request
Oct 17, 2023
`v88.5.4`⏩`v89.0.0` --- ## [`89.0.0`](https://github.com/elastic/eui/tree/v89.0.0) - Added new `pushAnimation` prop to push `EuiFlyout`s, which enables a slide in animation ([elastic#7239](elastic/eui#7239)) - Updated `EuiComboBox` to use `EuiInputPopover` under the hood ([elastic#7246](elastic/eui#7246)) - Added `inputPopoverProps` to `EuiComboBox`, which allows customizing the underlying popover ([elastic#7246](elastic/eui#7246)) - Added a new beta `EuiTextBlockTruncate` component for multi-line truncation ([elastic#7250](elastic/eui#7250)) - Updated `EuiBasicTable` and `EuiInMemoryTable` to support multi-line truncation. This can be set via `truncateText.lines` in the `columns` prop. ([elastic#7254](elastic/eui#7254)) **Bug fixes** - Fixed `EuiFlexGroup` and `EuiFlexGrid`'s `m` gutter size ([elastic#7251](elastic/eui#7251)) - Fixed focus trap rerender issues in `EuiFlyout` with memoization ([elastic#7259](elastic/eui#7259)) - Fixed a visual bug with `EuiContextMenu`'s animation between panels ([elastic#7268](elastic/eui#7268)) **Breaking changes** - EUI's global body font-size now respects the `font.defaultUnits` token. This means that the global font size will use the `rem` unit by default, instead of `px`. ([elastic#7182](elastic/eui#7182)) - Removed exported `accessibleClickKeys`, `comboBoxKeys`, and `cascadingMenuKeys` services. Use the generic `keys` service instead ([elastic#7256](elastic/eui#7256)) - Removed `EuiColorStops` due to low usage ([elastic#7262](elastic/eui#7262)) - Removed `EuiSuggest`. We recommend using `EuiSelectable` or `EuiComboBox` instead ([elastic#7263](elastic/eui#7263)) - Removed `euiHeaderAffordForFixed` Sass mixin, and `$euiHeaderHeight` and `$euiHeaderHeightCompensation` Sass variables. Use the CSS variable `--var(euiFixedHeadersOffset, 0)` instead. ([elastic#7264](elastic/eui#7264)) **Accessibility** - When using `rem` or `em` font units, EUI now respects, instead of ignoring, browser default font sizes set by end users. ([elastic#7182](elastic/eui#7182))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
closes #7192
🙈 I know this PR is huge, I'm sorry. I think most of it is just tests and snapshots. I tried to break commits up as atomically as I could, but unfortunately I couldn't see a clean place to split up the PR into smaller PRs without breaking functionality or making 1:1 conversions harder to follow.
The first and last commits are actually the only ones needed functionally (dogfooding
EuiInputPopoverand addinginputPopoverProps).However, it turns out there was a shit-ton of custom/duplicate logic in EuiComboBox's custom portalled dropdown menu (implemented before EuiInputPopover was created) that became functionally useless once we switched over. So a good 60%+ of this PR is me arduously going method by method through the component and its subcomponents and figuring out what could be safely yeeted and what couldn't. Turns out there's a lot of legacy code in EuiComboBox 💀
There was one EuiComboBox functionality (auto-closing the popover on scroll, added in #2106 / #2099) that I ported over to EuiInputPopover in 7e0cdc2 as an optional prop.
REVIEWER NOTE: The 2nd-to-last
TESTScommit is kind of a dumpster fire and I would recommend skimming it and not thoroughly reviewing. EuiComboBox tests are incredibly lacking and will almost certainly need to be at least partially rewritten in any case once we convert EuiComboBox to dogfood EuiSelectable, so the entire commit is basically a nice to have at best 🥲Screencaps
This entire massive refactor is really just for the last commit, so Kibana can do this:
QA
General checklist
@defaultif default values are missing) and playground toggles- [ ] Added documentation- Skipped as there isn't a lot of consumer-facing info to know- [ ] Checked Code Sandbox works for any docs examples- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)❓ I'm 98% sure there should not be any end-user-facing changes, and there also shouldn't be any consumer usage/prop changes. Please shout if you see otherwise