-
Notifications
You must be signed in to change notification settings - Fork 841
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 EuiFlexGroup, EuiFlexItem, and EuiFlexGrid #6270
Conversation
+ convert gutter CSS to `gap` + remove className maps + clean up unnecessary `...rest` cast + convert nested EuiFlexItem to flattened styles within flex item styles - realistically, who's going to be using EuiFlexItem outside of EuiFlexGroup?
…column directions - Due to switch to `gap`, no need for margin overrides anymore - the gap will be calculated automatically - Due to flatter Emotion classes, no need for !important anymore (which is probably extremely annoying to override) - see elastic#5529 for column directions decision
- separate out `grow` logic via JS to reduce CSS overrides - explicitly print grow-1 class if grow is a generic `true` + improve props table + remove `@ts-ignore` by switching type to ElementType + add missing `component` prop unit test
33e2587
to
f269f26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to highlight that I have intentionally removed all nesting as well as !important
declarations in our EuiFlexGroup CSS. IMO, this will be a significantly less annoying developer experience for our consumers, but may also have unintentional side effects where responsive CSS is now overridden by something else.
Please leave a comment if you feel the !important
s or high specificity needs to be preserved for any reason.
// sass-lint:disable-block no-important | ||
margin-left: 0 !important; | ||
margin-right: 0 !important; | ||
column-count: 1 !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above parent comment
.euiFlexItem { | ||
flex-grow: 1; | ||
flex-basis: 0%; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above parent comment
// On mobile we force them to stack and act the same. | ||
@include euiBreakpoint('xs', 's') { | ||
.euiFlexGroup--responsive > .euiFlexItem, | ||
.euiFlexGrid--responsive > .euiFlexItem { | ||
// sass-lint:disable-block no-important | ||
width: 100% !important; | ||
flex-basis: 100% !important; | ||
margin-left: 0 !important; | ||
margin-right: 0 !important; | ||
margin-bottom: $euiSize !important; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above parent comment
Preview documentation changes for this PR: https://eui.elastic.co/pr_6270/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6270/ |
@constancecchen I tested the changes and it looks good. 🎉 I found some minor issues. DocsWe can remove the following callout: Related issuesWhat do you think of documenting the suggested pattern (#2609 (comment)) in our docs: const isMobile = useIsWithinBreakpoints(['xs', 's']);
<EuiFlexGrid columns={isMobile ? 2 : 3} responsive={false}>
<EuiFlexItem>
<div>One</div>
</EuiFlexItem>
<EuiFlexItem>
<div>Two</div>
</EuiFlexItem>
<EuiFlexItem>
<div>Three</div>
</EuiFlexItem>
</EuiFlexGrid> Is this issue still valid? Or should we close it? I think this one can be closed. Right? |
80c6e60
to
92e4142
Compare
Awesome catches!! I totally forgot to do my normal docs pass, thanks Elizabet - will push fixes up here shortly.
✅ 92e4142
Looking into this, there's currently a responsive example for EuiFlexGroup so I think I'll need to do some shuffling around so the two examples aren't confused for each other. ✅ 3163f33 This is not closed by this issue. ❌ I think it would be better to have that be a separate/follow up PR, so we can more closely analyze the options that Yes, this PR fixes/closes the above issue - great catch. I've added that in the PR description and added a changelog ✅ 1163f84 |
+ move responsive flex group example up above grid examples
- combine src/components imports - remove unnecessary div wrappers
Preview documentation changes for this PR: https://eui.elastic.co/pr_6270/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6270/ |
- I probably should not have tried to tackle removing doc example Sass in this PR, sorry Elizabet 🙈
Preview documentation changes for this PR: https://eui.elastic.co/pr_6270/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @constancecchen! LGTM! 🎉
I tested locally in Chrome, Safari, Edge, and Firefox. I also tested using the playground.
Thanks! I'll open up a follow-up PR to address #4878 after this! |
## 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>
Summary
I strongly recommend following along by commit (due to the high amount of snapshot noise).
EuiFlexGroup
,EuiFlexGrid
, andEuiFlexItem
to EmotionEuiFlexItem
now supportsgrow={0}
EuiFlexGrid
no longer supportscolumns={0}
(use 1-4 or an EuiFlexGroup instead)EuiFlexGrid
now uses moderndisplay: grid
CSSEuiFlexGroup
,EuiFlexGrid
, andEuiFlexItem
now use moderngap
CSS instead of margins and negative marginsEuiFlexGroup
no longer applies responsive styles tocolumn
orcolumnReverse
directionscloses #5529
closes #4140
Things to look out for when moving styles
-inline
and-block
(Logical properties)gap
property to add margin between items if using flexdata
attribute instead.js
files be converted to TS? (e.g., docs files)- [ ] All animations or transitions are wrapped ineuiCanAnimate
- [ ] Convert global Sass vars to JS version like sizing- [ ] Convert component-specific Sass vars to exported JS versions- [ ] Delete any Amsterdam override .scss filesQA
shouldRenderCustomStyles()
passes with parent componentand any nestedchildProps
(e.g.tooltipProps, buttonProps
)Checklist
- [ ] Checked in both light and dark modes- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart