[EuiFilterSelectItem] Convert to Emotion + mark as deprecated by cee-chen · Pull Request #6982 · elastic/eui

cee-chen · 2023-07-24T14:45:26Z

Summary

This PR:

Converts EuiFilterSelectItem to Emotion
- This involves removing several CSS classNames that not being used directly within the EUI component, but instead as separate CSS hooks: .euiFilterSelect__items, .euiFilterSelect__note, and .euiFilterSelect__noteContent
Improves the keyboard "focus" state of disabled items - now uses a clearly disabled background color instead of the previous primary background color
Marks EuiFilterSelectItem for deprecation - once EuiComboBox switches to dogfooding EuiSelectable, we should move away from it entirely and request that Kibana usages do so as well

QA

Note that I opted not to create stories for this component since we'll eventually be removing it. The easiest way to QA it is to view its usage in EuiComboBox, and ensure combo boxes look the same as production: https://eui.elastic.co/pr_6982/#/forms/combo-box

General checklist

Checked in both light and dark modes
- [ ] Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
~~Added or~~ updated jest ~~and cypress tests~~
Checked for accessibility including keyboard-only ~~and screenreader~~ modes
A changelog entry exists and is marked appropriately

- [ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~
~~- [ ] Checked for breaking changes and labeled appropriately~~

Emotion checklist

Kibana usage

~~- [ ] Search Kibana's codebase for {euiComponent}- (case sensitive) to check for usage of modifier classes~~
~~- [ ] If usage exists, consider converting to a data attribute so that consumers still have something to hook into~~

Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding **/target, **/*.snap, **/*.storyshot for less noise) for eui{Component} (case sensitive) to find:
~~[ ] Any test/query selectors that will need to be updated~~ - None
~~[ ] Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted~~
Any direct className usages that will need to be refactored (e.g. someone calling the euiBadge class on a div instead of simply using the EuiBadge component)
- ⚠️ YES, there are multiple usages of this documented in the changelog that I will open a separate PR for.

General

Output CSS matches the previous CSS (works as expected in all browsers)
Rendered className(s) read as expected in snapshots and browsers
~~- [ ] Checked component playground~~ No playground/docs for this component

Unit tests

shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
Converted tests to RTL

Sass/Emotion conversion process

Removed component from src/components/index.scss
~~- [ ] Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)~~
~~- [ ] Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)~~
~~- [ ] Removed or converted component-specific Sass vars/mixins to exported JS versions~~
~~- [ ] Listed var/mixin removals in changelog~~
~~- [ ] Ran yarn compile-scss to update var/mixin JSON files~~
~~- [ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)~~
~~- [ ] Added an @warn deprecation message within the global_styling/mixins/{component}.scss file~~

CSS tech debt

Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)
~~- [ ] Wrapped all animations or transitions in euiCanAnimate~~
~~- [ ] Used gap property to add margin between items if using flex~~

DOM Cleanup

Did NOT remove any block/element classNames (e.g. euiComponent, euiComponent__child)
~~- [ ] SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.~~

Extras/nice-to-have

Reduced specificity where possible (usually by reducing nesting and class name chaining)
~~- [ ] Documentation pass:~~
~~- [ ] Check for issues in the backlog that could be a quick fix for that component~~
~~- [ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about~~

- simplify border

+ remove `-isFocused` class - no usages in Kibana

- not sure if this is already in prod, squash if so

- in favor of text truncation CSS util

- prefer using an actual component / `EuiSelectableMessage` instead

- replace with `.eui-yScroll` and custom `maxHeight` Emotion CSS - there are no usages within EUI, but 15 usages within Kibana - we'll have to convert manually and ask consumers to

cee-chen · 2023-07-24T18:55:56Z

@breehall Sending this your way for review since you're on release/Kibana upgrade duty this week, and I'm wanting to get this conversion in before tomorrow's release (+ will be updating your Kibana upgrade PR). For the most part I'm hoping everything should be follow-able by commit, but please let me know if not or if anything is unclear!

kibanamachine · 2023-07-24T18:58:14Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6982/

- Fix mounted test affected by new class/emotion wrapper (convert to RTL) - update snapshots

kibanamachine · 2023-07-24T19:53:39Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6982/

breehall

Hey Cee, this looks good! Just two questions/considerations below for you. I checked this out in prod and everything looks good.

Since this component is slated to be deprecated in December of this year, do we have a timeframe for converting the guts of EuiComboBox to EuiSelectable? We may want to add this to our team roadmap for upcoming work.

breehall · 2023-07-25T14:54:18Z

src/components/filter_group/filter_select_item.styles.ts

+    isFocused: css`
+      ${focusStyles}


If we removed the isFocus class because it had no usages, do we need this?

Yep, we still need this - the className wasn't being referenced directly in Kibana, but EUI still applies isFocused styles manually. You can see this in EuiComboBox when using arrow keys to navigate through options - those aren't :focus styles, they're isFocused.

breehall · 2023-07-25T15:10:01Z

src/components/filter_group/_filter_select_item.scss

-.euiFilterSelect__items {
-  @include euiScrollBar;
-
-  overflow-y: auto;
-  max-height: $euiSize * 30;
-}


Since there are 15 usages in Kibana and this component has already been slated for deprecation, do you think there would be merit in adding these styles an exportable style from Emotion? My thinking is this could simplify the process of manually adding the class and the height. Adding it manually would be a very small change either way, but I'm curious on your thoughts.

I'd rather leave cruft in Kibana than EUI I've already done the work locally in Kibana to convert all usages of this to className="eui-yScroll" css={{ maxHeight: euiTheme.base * 30 }}, and also added a long comment also asking devs to switch to EuiSelectable which already has scrolling/max-height baked in OOTB.

Totally valid! Let's go with it!

breehall · 2023-07-25T15:12:31Z

src/components/combo_box/combo_box.test.tsx

+  fireEvent.focus(getByTestSubject('comboBoxSearchInput'));
+
+  const dropdownOptions = baseElement.querySelectorAll('.euiFilterSelectItem');
+  expect(
+    dropdownOptions[0]!.querySelector('[data-euiicon-type="check"]')
+  ).toBeFalsy();
+  expect(
+    dropdownOptions[1]!.querySelector('[data-euiicon-type="check"]')
+  ).toBeTruthy();


breehall

Thanks for tackling my questions! This is good to go! 🚢

cee-chen · 2023-07-25T17:00:19Z

Since this component is slated to be deprecated in December of this year, do we have a timeframe for converting the guts of EuiComboBox to EuiSelectable? We may want to add this to our team roadmap for upcoming work.

Haha whoops, I definitely yolo'd the deprecation date for December and forgot to mention it's a tentative month I chose semi-randomly. We're free to punt on that deprecation date if we don't convert by then, but I'm definitely hoping to get to it before then (+ honestly, it makes a lot of sense to do before EuiComboBox's Emotion conversion).

cee-chen · 2023-07-25T17:02:33Z

I updated the deprecation meta issue to note that #2708 is required and the deprecation date can be moved if it's not yet complete. Thanks Bree!

`85.0.0` ➡️ `85.1.0` ⚠️ The biggest change in this PR by far is the **removal** of several `EuiFilterSelectItem` CSS classes and styles associated with those classes. EUI has previously exported component-specific CSS classes for usage such as `.euiFilterSelect__items`, `.euiFilterGroup__popoverPanel`, or `.euiAccordionForm`. ❌ **As of our move to CSS-in-JS, we are actively moving away from this architecture**. The goal is to either provide either a component or prop directly to you instead of a CSS class. As a reminder, our classNames are not guaranteed APIs and are subject to change without warning - should you need to tweak or customize EUI's styling, we strongly recommend passing in your own `className`. 💬 Moving forward, EUI will attempted to convert any usages of removed CSS classes and their direct usages in Kibana for you. In most cases, we'll hopefully be able to take the correct path of using a component or prop instead. In cases where we cannot or significant/complex changes are required, we may temporarily convert the CSS to a static CSS-in-JS usage instead and add a TODO asking the relevant team to address this in their own time (for example, the deprecation of `EuiFilterSelectItem` and its conversion to `EuiSelectable`). ## [`85.1.0`](https://github.com/elastic/eui/tree/v85.1.0) - Updated `EuiComboBox`'s `options` to accept `option.append` and `option.prepend` props ([#6953](elastic/eui#6953)) - Updated deprecated `.substr()` usages to `.substring()` ([#6954](elastic/eui#6954)) - Updated `EuiInlineEdit`'s read mode button to include a title tooltip, increasing readability of truncated text ([#6966](elastic/eui#6966)) **Bug fixes** - Fixed `EuiFilterGroup`'s responsive styles ([#6983](elastic/eui#6983)) **Deprecations** - Deprecated `EuiFilterSelectItem`; Use `EuiSelectable` instead ([#6982](elastic/eui#6982)) **CSS-in-JS conversions** - Converted `EuiFilterSelectItem` to Emotion ([#6982](elastic/eui#6982)) - Removed `.euiFilterSelect__items` CSS; Use `EuiSelectable` instead ([#6982](elastic/eui#6982)) - Removed `.euiFilterSelect__note` and `.euiFilterSelect__noteContent` CSS; Use `EuiSelectableMessage` instead ([#6982](elastic/eui#6982)) - Added `focus.transparency` and `focus.backgroundColor` theme tokens ([#6984](elastic/eui#6984)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cee-chen changed the title ~~[Emotion] Add focus.transparency & focus.backgroundColor theme tokens; Convert EuiFilterSelectItem to Emotion; Deprecate EuiFilterSelectItem~~ [Emotion] Convert EuiFilterSelectItem + set up required focus theme tokens Jul 24, 2023

This was referenced Jul 24, 2023

[Emotion] EuiFilterGroup #6398

Closed

[EuiFilterGroup] Fix responsive styles #6983

Merged

cee-chen force-pushed the emotion/filter-group-select-item branch from 3084e10 to 6020d02 Compare July 24, 2023 18:28

cee-chen added 11 commits July 24, 2023 11:33

[EuiFilterSelectItem] Basic styles setup

081f1a1

Update tests

426c848

Fix EuiComboBox type references

d9bc8c8

Convert more Sass styles

c6b39d5

- simplify border

Convert interactive styles

d7ba3e6

+ remove `-isFocused` class - no usages in Kibana

Improve disabled focused styles

affe016

- not sure if this is already in prod, squash if so

Remove .euiFilterSelectItem__content styles

0bd419a

- in favor of text truncation CSS util

Remove unused styles only attached to a className

4d4dd6c

- prefer using an actual component / `EuiSelectableMessage` instead

Remove .euiFilterSelect__items CSS

457022f

- replace with `.eui-yScroll` and custom `maxHeight` Emotion CSS - there are no usages within EUI, but 15 usages within Kibana - we'll have to convert manually and ask consumers to

Remove Sass files

788b8f5

Set deprecation notice for EuiFilterSelectItem

5a9d894

cee-chen force-pushed the emotion/filter-group-select-item branch from 6020d02 to 781e57b Compare July 24, 2023 18:33

changelog

abadb53

cee-chen changed the title ~~[Emotion] Convert EuiFilterSelectItem + set up required focus theme tokens~~ [EuiFilterSelectItem] Convert to Emotion + mark as deprecated Jul 24, 2023

cee-chen marked this pull request as ready for review July 24, 2023 18:53

cee-chen requested a review from breehall July 24, 2023 18:53

cee-chen force-pushed the emotion/filter-group-select-item branch from 781e57b to 4bf91d4 Compare July 24, 2023 19:16

Fix/update downstream EuiComboBox tests

7f4a786

- Fix mounted test affected by new class/emotion wrapper (convert to RTL) - update snapshots

cee-chen force-pushed the emotion/filter-group-select-item branch from 4bf91d4 to 7f4a786 Compare July 24, 2023 19:29

breehall reviewed Jul 25, 2023

View reviewed changes

breehall approved these changes Jul 25, 2023

View reviewed changes

cee-chen merged commit 26a5d3d into elastic:main Jul 25, 2023

cee-chen deleted the emotion/filter-group-select-item branch July 25, 2023 17:03

breehall mentioned this pull request Jul 27, 2023

Upgrade EUI to v85.1.0 elastic/kibana#162660

Merged

weronikaolejniczak mentioned this pull request Mar 5, 2025

[EuiFilterSelectItem] Investigate the usage and plan appropriate action regarding deprecation #8395

Closed

3 tasks

Conversation

cee-chen commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Emotion checklist

Uh oh!

cee-chen commented Jul 24, 2023

Uh oh!

kibanamachine commented Jul 24, 2023

Uh oh!

kibanamachine commented Jul 24, 2023

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

breehall Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breehall Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jul 25, 2023

Uh oh!

cee-chen commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cee-chen commented Jul 24, 2023 •

edited

Loading

cee-chen Jul 25, 2023 •

edited

Loading