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.

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

[Emotion] Convert EuiListGroup #6207

Merged
merged 40 commits into from
Sep 21, 2022
Merged

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Sep 6, 2022

Summary

This PR converts EuiListGroup and EuiListGroupItem to Emotion. It also updates the designs to make them more similar to what we have in EUI Figma:

Screenshot 2022-09-13 at 12 41 25

What are the changes?

  • The hover and focus background matches the item color. In the past list items with color="primary" would have on active or on hover a light gray background. Now they have a light blue background. When color="text" or color="subdued" the background for active and hover states is a light gray.

list_group_before_after@2x

  • The inherit color was removed. It doesn't make sense. When we're using a list group most likely we want all the list items to behave similarly and have similar colors to be consistent. As we can see on the following CollapsibleNavGroup
  • The extra actions now inherit the color of the item. Once the item is pinned it gets gray.

collapsible_nav_before_after@2x

Breaking changes

  • Removed ghost color from EuiListGroupItem. Consumers now must pass a EuiThemProvider color="dark" to use a ghost color.
  • Removed inherit color from EuiListGroupItem. The inherit in my opinion was not making sense anymore. The consumer should pass the color they think makes sense for their use case.
  • The list items now defaults to color text

I searched all the Elastic organization for color="inherit" and I couldn't find anything.

Searched for color="ghost":

Both instances need to be updated to contain a EuiThemeProvider colorMode="dark".

I also found 4 instances on different Elastic projects where EuiListGroupItem are passing href this means by default they were inheriting a primary color but with a light gray background on active or hover. And now the link will have a text color.

So consumers can pass color="primary" to EuiListGroup or keep the color text as it is. I don't see this as a major breaking change. And probably there is more usages that I might be missing.

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

Things to look out for when moving styles

  • Anything in the backlog that could be a quick fix for that component?
  • Convert global Sass vars to JS version like sizing
  • Convert component-specific Sass vars to exported JS versions
  • Convert side specific padding, margin, and position to -inline and -block (Logical properties)
  • Reduce specificity where possible (usually by reducing class name chaining)
  • Use gap property to add margin between items if using flex
  • Can any still existing .js files be converted to TS?
  • All animations or transitions are wrapped in euiCanAnimate

QA

  • Does the output CSS match the previous CSS / as expected
  • Does the rendered class read as expected

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

…tionals & obj keys

+ DRY out repeated euiBackgroundColor() calls
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

…arly in props table

+ move alwayShow to top of props list
- `.euiListGroupItem-isDisabled` is no longer a className, so the :not() selector is not working and the action still appears on hover on disabled items

- passing `parentIsDisabled` as an internal prop from EuiListGroupItem and making it an array conditional solves the hover issue

- the parent li is not focusable so the `:focus &` selector doesn't work either. Changing it to to the button and `+ &` solves the issue

+ remove unnecessary comment
…array further

- the hover CSS is unnecessary if alwaysShow is true, so add a condition for it
@kibanamachine
Copy link

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

- currently 8px on prod which generally seems to match the button padding size better
- this allows button content inside tooltips to grow to 100% of the EuiListGroupItem
- this is an enhancement from current prod behavior
- Remove unnecessary CSS on __inner - the width: 100% is totally unnecessary (tested with both extra action and without) and the word break can be applied to the __label CSS instead

- DRY out unsetting CSS by using ternary instead of base CSS (but keep empty base for naming)

- DRY out CSS by using text utils
- use logical CSS property instead of max-width

- do not apply maxWidthDefault emotion style if using a custom inline max-width

- fix maxWidth conditional checks

- remove || fallback / type def in favor of let

- improve props documentation/typing
+ test nested `iconProps` and `extraAction` props

+ fix `extraAction` props not correctly passing/merging `className`

+ fix misc console.warn about missing EuiButtonIcon aria-label

+ add test for `style` prop on parent EuiListGroupItem, since it gets spread to the wrapped child and not the className wrapper
- they're now being correctly passed to the `EuiListGroupItemExtraAction` but they're not really doing anything / just adding a lot of noise

- consumers can simply hook into `.euiListGroupItemExtraAction` instead

- no Kibana references to these classes, so IMO they're safe to remove
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

That's all my feedback! LMK if you have any questions on any changes, I highlighted one that I wasn't sure about.

euiListGroupItemExtraAction: css`
flex-shrink: 0;
opacity: 0;
${logicalCSS('margin-right', euiTheme.size.s)};
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I changed this to s size (8px) down from m (12px) to match production. I wasn't sure if m was an intentional change but visually it looked a little weird/large to me. Feel free to revert this back to m if that was an intentional change however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing. It was not intentional.

Copy link
Contributor Author

@elizabetdev elizabetdev Sep 21, 2022

Choose a reason for hiding this comment

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

Thanks, @constancecchen for all the feedback.

I'm merging the PR. I tested in Chrome, Safari, and Firefox. All your changes look good! 🎉

@kibanamachine
Copy link

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

@elizabetdev elizabetdev merged commit deabfa9 into elastic:main Sep 21, 2022
jbudz pushed a commit to elastic/kibana that referenced this pull request Nov 18, 2022
## Summary
`[email protected]` ⏩ `[email protected]`

⚠️ Note: This upgrade contains breaking changes to `EuiFlexGroup` and
`EuiFlexGrid`, primarily around switching margins and negative margins
to `gap`. Please do a quick QA pass of your app to scan for any issues.
We're happy to help resolve minor fixes, or potentially follow up after
PR merges. You can find us over in #eui!

## [`70.2.4`](https://github.com/elastic/eui/tree/v70.2.4)

**Bug fixes**

- Fixed visual bug in nested `EuiFlexGroup`s, where the parent
`EuiFlexGroup` is responsive but a child `EuiFlexGroup` is not
([#6381](elastic/eui#6381))

## [`70.2.3`](https://github.com/elastic/eui/tree/v70.2.3)

**Bug fixes**

- Fixed incorrect margins in `EuiSuperDatePicker` caused by `EuiFlex`
CSS gap change ([#6380](elastic/eui#6380))

## [`70.2.2`](https://github.com/elastic/eui/tree/v70.2.2)

- `EuiButton` now accepts `minWidth={false}`
([#6373](elastic/eui#6373))

**Bug fixes**

- `EuiButton` no longer outputs unnecessary inline styles for
`minWidth={0}` or `minWidth={false}`
([#6373](elastic/eui#6373))
- `EuiFacetButton` no longer reports type issues when passing props
accepted by `EuiButton`
([#6373](elastic/eui#6373))
- Fixed the shadow sizes of `.eui-yScrollWithShadows` and
`.eui-xScrollWithShadows`
([#6374](elastic/eui#6374))


## [`70.2.1`](https://github.com/elastic/eui/tree/v70.2.1)

**Bug fixes**

- Re-fixed `EuiPageSection` not correctly merging `contentProps.css`
([#6365](elastic/eui#6365))
- Fixed `EuiTab` not defaulting to size `m`
([#6366](elastic/eui#6366))

## [`70.2.0`](https://github.com/elastic/eui/tree/v70.2.0)

- Added a keyboard shortcuts popover to `EuiDataGrid`'s toolbar. This
can be visually hidden via `toolbarVisibility.showKeyboardShortcuts`,
but will always remain accessible to keyboard and screen reader users.
([#6036](elastic/eui#6036))
- `EuiScreenReaderOnly`'s `showOnFocus` prop now also shows on focus
within its children ([#6036](elastic/eui#6036))
- Added `onFocus` prop callback to `EuiSuperDatePicker`
([#6320](elastic/eui#6320))

**Bug fixes**

- Fixed `EuiSelectable` to ensure the full options list is re-displayed
when the search bar is controlled and cleared using `searchProps.value`
([#6317](elastic/eui#6317))
- Fixed incorrect padding on `xl`-sized `EuiTabs`
([#6336](elastic/eui#6336))
- Fixed `EuiCard` not correctly merging `css` on its child `icon`s
([#6341](elastic/eui#6341))
- Fixed `EuiCheckableCard` not setting `css` on the correct DOM node
([#6341](elastic/eui#6341))
- Fixed a webkit rendering issue with `EuiModal`s containing
`EuiBasicTable`s tall enough to scroll
([#6343](elastic/eui#6343))
- Fixed bug in `to_initials` that truncates custom initials
([#6346](elastic/eui#6346))
- Fix bug in `EuiCard` where layout breaks when `horizontal` and
`selectable` are both passed
([#6348](elastic/eui#6348))

## [`70.1.0`](https://github.com/elastic/eui/tree/v70.1.0)

- Added the `hint` prop to the `<EuiSearchBar />`. This prop lets the
consumer render a hint below the search bar that will be displayed on
focus. ([#6319](elastic/eui#6319))
- Added the `hasDragDrop` prop to `EuiPopover`. Use this prop if your
popover contains `EuiDragDropContext`.
([#6329](elastic/eui#6329))

**Bug fixes**

- Fixed `EuiButton`'s cursor style when the button is disabled
([#6323](elastic/eui#6323))
- Fixed `EuiPageTemplate` not recognizing child
`EuiPageSidebar`s/`EuiPageTemplate.Sidebar`s with `css` props
([#6324](elastic/eui#6324))
- Fixed `EuiBetaBadge` to always respect its `anchorProps` values,
including when there is no tooltip content
([#6326](elastic/eui#6326))
- Temporarily patched `EuiModal` to not cause scroll-jumping issues on
modal open ([#6327](elastic/eui#6327))
- Fixed buggy drag & drop behavior within `EuiDataGrid`'s columns &
sorting toolbar popovers
([#6329](elastic/eui#6329))
- Fixed `EuiButton` not correctly passing `textProps` for children
inside fragments or i18n components
([#6332](elastic/eui#6332))
- Fixed `EuiButton` not correctly respecting `minWidth={0}`
([#6332](elastic/eui#6332))

**CSS-in-JS conversions**

- Converted `EuiTabs` to Emotion
([#6311](elastic/eui#6311))

## [`70.0.0`](https://github.com/elastic/eui/tree/v70.0.0)

- Added the `enabled` option to the `<EuiInMemoryTable />`
`executeQueryOptions` prop. This option prevents the Query from being
executed when controlled by the consumer.
([#6284](elastic/eui#6284))

**Bug fixes**

- Fixed `EuiOverlayMask` to set a
`[data-relative-to-header=above|below]` attribute to replace the
`--aboveHeader` and `--belowHeader` classNames removed in its Emotion
conversion ([#6289](elastic/eui#6289))
- Fixed `EuiHeader` CSS using removed `EuiOverlayMask` class modifiers
([#6293](elastic/eui#6293))
- Fixed `EuiToolTip` not respecting reduced motion preferences
([#6295](elastic/eui#6295))
- Fixed a bug with `EuiTour` where passing any `panelProps` would cause
the beacon to disappear
([#6298](elastic/eui#6298))

**Breaking changes**

- `@emotion/css` is now a required peer dependency, alongside
`@emotion/react` ([#6288](elastic/eui#6288))
- `@emotion/cache` is no longer required peer dependency, although your
project must still use it if setting custom cache/injection locations
([#6288](elastic/eui#6288))

**CSS-in-JS conversions**

- Converted `EuiCode` and `EuiCodeBlock` to Emotion; Removed
`euiCodeSyntaxTokens` Sass mixin and `$euiCodeBlockPaddingModifiers`;
([#6263](elastic/eui#6263))
- Converted `EuiResizableContainer` and `EuiResizablePanel` to Emotion
([#6287](elastic/eui#6287))

## [`69.0.0`](https://github.com/elastic/eui/tree/v69.0.0)

- Added support for `fullWidth` prop on EuiForm, which will be the
default for all rows/controls within
([#6229](elastic/eui#6229))
- Added support for `onResizeStart` and `onResizeEnd` callbacks to
`EuiResizableContainer`
([#6236](elastic/eui#6236))
- Added optional case sensitive option matching to `EuiComboBox` with
the `isCaseSensitive` prop
([#6268](elastic/eui#6268))
- `EuiFlexItem` now supports `grow={0}`
([#6270](elastic/eui#6270))
- Added the `alignItems` prop to `EuiFlexGrid`
([#6281](elastic/eui#6281))
- Added `filter`, `filterExclude`, `filterIgnore`, `filterInclude`,
`indexTemporary`, `infinity`, `sortAscending`, and `sortDescending`
glyphs to `EuiIcon` ([#6282](elastic/eui#6282))

**Bug fixes**

- Fixed `EuiTextProps` to show the `color` type option `inherit` as
default ([#6267](elastic/eui#6267))
- `EuiFlexGroup` now correctly respects `gutterSize` when responsive
([#6270](elastic/eui#6270))
- Fixed the last breadcrumb in `EuiBreadcrumbs`'s `breadcrumbs` array
not respecting `truncate` overrides
([#6280](elastic/eui#6280))

**Breaking changes**

- `EuiFlexGrid` no longer supports `columns={0}`. Use `EuiFlexGroup`
instead for normal flex display
([#6270](elastic/eui#6270))
- `EuiFlexGrid` now uses modern `display: grid` CSS
([#6270](elastic/eui#6270))
- `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` now use modern `gap`
CSS instead of margins and negative margins
([#6270](elastic/eui#6270))
- `EuiFlexGroup` no longer applies responsive styles to `column` or
`columnReverse` directions
([#6270](elastic/eui#6270))

**CSS-in-JS conversions**

- Converted `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` to Emotion
([#6270](elastic/eui#6270))

## [`68.0.0`](https://github.com/elastic/eui/tree/v68.0.0)

- Added `beta` glyph to `EuiIcon`
([#6250](elastic/eui#6250))
- Added `launch` and `spaces` glyphs to `EuiIcon`
([#6260](elastic/eui#6260))
- Added the `fallbackDestination` prop to `EuiSkipLink`, which accepts a
string of query selectors to fall back to if the `destinationId` does
not have a valid target. Defaults to `main`
([#6261](elastic/eui#6261))
- `EuiSkipLink` is now always an `a` tag to ensure that it is always
placed within screen reader link menus.
([#6261](elastic/eui#6261))

**Bug fixes**

- Fixed `EuiSuperDatePicker` not correctly merging passed `className`s
([#6253](elastic/eui#6253))
- Fixed `EuiColorStops` not correctly merging in passed
`data-test-subj`s, `style`s, or `...rest`
([#6255](elastic/eui#6255))
- Fixed `EuiResizablePanel` incorrectly passing `style` to the wrapper
instead of the panel. Use `wrapperProps.style` to pass styles to the
wrapper. ([#6255](elastic/eui#6255))
- Fixed custom `onClick`s passed to `EuiSkipLink` overriding
`overrideLinkBehavior`
([#6261](elastic/eui#6261))

**Breaking changes**

- Removed `inherit` and `ghost` color from `EuiListGroupItem`
([#6207](elastic/eui#6207))
- Changed default color to `text` instead of `inherit`
([#6207](elastic/eui#6207))

**CSS-in-JS conversions**

- Converted `EuiListGroup` and `EuiListGroupItem` to Emotion; Removed
`$euiListGroupGutterTypes`, `$euiListGroupItemColorTypes` and
`$euiListGroupItemSizeTypes`;
([#6207](elastic/eui#6207))
- Converted `EuiBadgeGroup` to Emotion
([#6258](elastic/eui#6258))
- Converted `EuiBetaBadge` to Emotion
([#6258](elastic/eui#6258))
- Converted `EuiNotificationBadge` to Emotion
([#6258](elastic/eui#6258))

Co-authored-by: Elizabet Oliveira <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants