[EuiButton] Fix multiple issues around internal EuiButtonDisplay components #6332

cee-chen · 2022-10-28T22:32:20Z

Summary

This PR primarily closes #6310 but also cleans up and addresses other issues found while writing unit tests. I recommend following along by commit.

Fixes multiple bugs around EuiButtonDisplay and EuiButtonDisplayContent (see changelog and individual commits)
Fixes an inline style property to use its logical counterpart
Cleans up unnecessary & unused CSS
Writes missing unit tests for EuiButtonDisplay and EuiButtonDisplayContent
Fixes EuiLoadingSpinner to not potentially generate infinite Emotion classes per wildcard color prop but instead use inline styles

QA

General checklist

Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress tests
A changelog entry exists and is marked appropriately

…/empty objs

- more accessible to rtl

- flex shrink is already set by EuiLoadingSpinner and EuiIcon respectively and does not need to be repeated - size CSS isn't doing anything and isn't worth keeping

- we should be using inline styles for non-enum props to avoid generating n/infinite Emotion classNames + add unit test for EuiButtonDisplayContent & remove unnecessary typing

kibanamachine · 2022-10-28T23:02:10Z

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

breehall

Reviewed commit by commit and I love the new tests that are included. They're a lot more specific. Pulled branch and walked through test cases locally. Confirmed changes in UI and also confirmed that this resolves the icon text toggle issue we saw before.

breehall · 2022-10-28T23:40:58Z

src/components/button/button_display/_button_display_content.tsx

@@ -110,7 +107,7 @@ export const EuiButtonDisplayContent: FunctionComponent<

  return (
    <span css={cssStyles} {...contentProps}>
-      {icon}
+      {iconSide === 'left' && icon}


This change makes a lot of sense! I like it. From what I can see in Kibana, the only things referencing the the side in the icon class name are snapshots, so I believe we're safe to move forward with this.

miukimiu

Thanks, @constancecchen,

I also reviewed the PR commit by commit. I tested the super date picker example with the icon-only toggle and can confirm that #6310 seems fixed.

Tested the buttons in the playground. I also tested in Chrome, Safari, and Firefox.

## Summary `eui@67.1.8` ⏩ `eui@70.2.2` ⚠️ 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 <elizabet.oliveira@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…onents (elastic#6332) * Fix text wrapper not being used for EuiI18n 'text' that pass textProps * Fix unnecessary textProps spread causing false positive for undefined/empty objs * Fix minWidth inline style not applying for 0px + use logical property * [opinionated] iconSide change: prefer DOM order over CSS order - more accessible to rtl * [cleanup] remove unnecessary/unused CSS - flex shrink is already set by EuiLoadingSpinner and EuiIcon respectively and does not need to be repeated - size CSS isn't doing anything and isn't worth keeping * Write more unit tests for EuiButtonDisplayContent * Fix EuiLoadingSpinner to not pass wildcard color values into Emotion - we should be using inline styles for non-enum props to avoid generating n/infinite Emotion classNames + add unit test for EuiButtonDisplayContent & remove unnecessary typing * write more/missing unit tests for logic within internal EuiButtonDisplay * changelog

cee-chen added 8 commits October 28, 2022 11:29

Fix text wrapper not being used for EuiI18n 'text' that pass textProps

210e1d6

Fix unnecessary textProps spread causing false positive for undefined…

5bef472

…/empty objs

Fix minWidth inline style not applying for 0px + use logical property

8252261

[opinionated] iconSide change: prefer DOM order over CSS order

a0c2e47

- more accessible to rtl

[cleanup] remove unnecessary/unused CSS

686b797

- flex shrink is already set by EuiLoadingSpinner and EuiIcon respectively and does not need to be repeated - size CSS isn't doing anything and isn't worth keeping

Write more unit tests for EuiButtonDisplayContent

13d9805

Fix EuiLoadingSpinner to not pass wildcard color values into Emotion

c56336f

- we should be using inline styles for non-enum props to avoid generating n/infinite Emotion classNames + add unit test for EuiButtonDisplayContent & remove unnecessary typing

write more/missing unit tests for logic within internal EuiButtonDisplay

8b51bd7

cee-chen requested review from breehall and miukimiu October 28, 2022 22:32

changelog

0fdfb69

cee-chen marked this pull request as ready for review October 28, 2022 22:35

breehall approved these changes Oct 29, 2022

View reviewed changes

miukimiu approved these changes Oct 31, 2022

View reviewed changes

cee-chen merged commit e04d47e into elastic:main Oct 31, 2022

cee-chen deleted the button-fixes branch October 31, 2022 17:02

cee-chen mentioned this pull request Oct 31, 2022

Bumping EUI to version 67.1.8 elastic/kibana#141279

Merged

cee-chen mentioned this pull request Nov 8, 2022

Upgrade EUI to v70.2.4 elastic/kibana#144845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiButton] Fix multiple issues around internal EuiButtonDisplay components #6332

[EuiButton] Fix multiple issues around internal EuiButtonDisplay components #6332

cee-chen commented Oct 28, 2022 •

edited

kibanamachine commented Oct 28, 2022

breehall left a comment

breehall Oct 28, 2022

miukimiu left a comment

[EuiButton] Fix multiple issues around internal EuiButtonDisplay components #6332

[EuiButton] Fix multiple issues around internal EuiButtonDisplay components #6332

Conversation

cee-chen commented Oct 28, 2022 • edited

Summary

QA

General checklist

kibanamachine commented Oct 28, 2022

breehall left a comment

Choose a reason for hiding this comment

breehall Oct 28, 2022

Choose a reason for hiding this comment

miukimiu left a comment

Choose a reason for hiding this comment

cee-chen commented Oct 28, 2022 •

edited