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.

Sign up for GitHub

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

Jump to bottom

[EuiTabs] More Emotion cleanup #6336

Merged
merged 11 commits into from
Nov 7, 2022
Merged

[EuiTabs] More Emotion cleanup #6336

merged 11 commits into from
Nov 7, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Nov 1, 2022 •
edited
Loading

Summary

I didn't get to #6311 in time to push up some code cleanup, so opening a follow-up PR. I recommend following along by commit.

QA

General checklist

Emotion conversion checklist

  • Does it work?
  • Output CSS matches the previous CSS / as expected in browsers
  • Rendered className reads as expected in snapshots and in browsers
     
  • Unit tests
  • shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
  • Removed any mount()ed snapshots in favor of render() or a more specific assertion
     
  • Sass/Emotion conversion process
  • Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)
     
  • CSS tech debt
  • Reduced specificity where possible (usually by reducing nesting and class name chaining)
  • Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)
     
  • DOM cleanup
  • Did not remove any block/element classNames (e.g. euiComponent, euiComponent__child)

@cee-chen
Remove mounted snapshot
bf4b747
@cee-chen
Emotion cleanup - remove unnecessary calcs
0559db6
@cee-chen
CSS cleanup - remove incorrect text color and unnecessary extra hover…
72e9fed 
… styles

- text color is set by `euiTitle` and should not be disabled by default

- disabled already inherits hover disabling from the default
@cee-chen
CSS - misc logical property conversion
b50a816 
- default shorthands sadly do not account for logical/direction changes
@cee-chen
Flatten/fix disabled selected selector
2033da5
@cee-chen
Revert removed __append/prepend classNames
9ed121a 
- we're generally not removing `__` classNames in our conversion to give consumers something to hook onto
@cee-chen
DRY out tab sizes types/consts
d7f612e 
+ fix type name to match type name pre-Emotion conversion
@cee-chen
Add missing shouldRenderCustomStyles tests to sub components
56da139
@cee-chen
[misc] rename Emotion vars to match other components/conversions
bb66f3f
@cee-chen
Remove unnecessary padding sizes on euiTab
39712c9 
Per old Amsterdam overrides, *all* padding for all sizes should be the same (0 xs)
@cee-chen
changelog
3e8a548
@cee-chen cee-chen requested review from elizabetdev and daveyholler and removed request for elizabetdev November 1, 2022 20:20
cee-chen
cee-chen commented Nov 1, 2022
View reviewed changes
src/components/tabs/tab.styles.ts
Comment on lines -28 to -40
// sizes
s: css`
padding: 0 ${euiTheme.size.xs};
`,
m: css`
padding: 0 ${euiTheme.size.xs};
`,
l: css`
padding: 0 ${euiTheme.size.xs};
`,
xl: css`
padding: ${euiTheme.size.s} ${euiTheme.size.xs};
`,
Copy link
Member Author

@cee-chen cee-chen Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@miukimiu stop me if I'm reading the old amsterdam overrides SCSS wrong but it looked to me like in Amsterdam all paddings were supposed to be 0 xs and as such we don't need a sizes modifier.

The playground in https://eui.elastic.co/v67.0.0/#/navigation/tabs (pre-Emotion-conversion) appears to confirm this

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirmed by looking into the old file and you're right.

@cee-chen cee-chen marked this pull request as ready for review November 1, 2022 20:24
@kibanamachine
Copy link

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

elizabetdev
elizabetdev approved these changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Tested in Chrome, Safari, Edge, and Firefox. I also looked at the code. LGTM! 🎉

@cee-chen
Copy link
Member Author

cee-chen commented Nov 7, 2022

Thanks a million @miukimiu!

@cee-chen cee-chen merged commit 83d7a6d into elastic:main Nov 7, 2022
@cee-chen cee-chen deleted the tabs-cleanup branch November 7, 2022 15:45
@cee-chen cee-chen mentioned this pull request Nov 8, 2022
Merged
jbudz pushed a commit to elastic/kibana that referenced this pull request Nov 18, 2022
@elizabetdev @kibanamachine
Upgrade EUI to v70.2.4 (#144845)
c408ceb 
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elizabetdev elizabetdev elizabetdev approved these changes

@daveyholler daveyholler Awaiting requested review from daveyholler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cee-chen @kibanamachine @elizabetdev