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 EuiHeaderAlert, EuiHeaderSection, EuiHeaderSectionItem, EuiHeaderSectionItemButton, and EuiHeaderLinks #7005

Merged
merged 25 commits into from
Aug 1, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jul 27, 2023

Summary

Continuation of #6878, closes #6417

As always, I recommend following along by commit - this may seem like a lot but it's mostly storybooks 🤞 (it's helpful to follow along with storybook open!) I also tried to make commits as atomic as possible, so it looks like there's a lot, but they mostly go by quick.

Breaking changes/removals:

  • Removed border prop from EuiHeaderSectionItem (hasn't been doing anything since the Amsterdam theme, might as well remove it while we're here)
  • Removed borders object configuration from EuiHeader.sections (same as above)
  • Removed unused .euiHeaderAlert__dismiss CSS (✅ No usages in either EUI or Kibana)
  • Removed $euiHeaderLinksGutterSizes Sass variables and modifier classes (✅ No usages in Kibana)
  • Removed $euiHeaderBackgroundColor Sass variable; use $euiColorEmptyShade instead (⚠️ 1 usage in Kibana)
  • Removed $euiHeaderChildSize Sass variable; use $euiSizeXXL instead (⚠️ 1 usage in Kibana - it's mine lol)

QA

  • Compare staging against production and confirm that all demos look the same as before
  • gh pr checkout 7005
  • yarn storybook
  • Go to the EuiHeaderAlert stories
    • Confirm that the single alert item and multiple alert items within flyouts and popovers look good in light & dark mode
  • Go to the EuiHeaderLinks story
    • Confirm the gutterSize controls work as expected
    • Change one of the popoverBreakpoints to l/xl (whatever fits your screen) and confirm the links collapse down into a single button with a popover, and links look as expected within the popover
  • Go to the EuiHeaderSection story
    • Confirm the side control works as expected
    • Confirm the grow control works as expected (may require inspecting the item in the DOM)
  • Go to the EuiHeaderSectionItem story
    • There's nothing really to do here 😅 Confirm it looks as expected
  • Go to the EuiHeaderSectionItemButton story
    • (Playground) Toggle between notification true/false
    • (Notification) Toggle between notificationColors and custom notification content
    • (Animation) Click the Play animation button and confirm it works as expected

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 breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately
    - [ ] Updated the Figma library counterpart - There doesn't seem to be specific components for this in Figma, it just reuses EuiButtonEmpty / EuiBUttonIcon

Emotion checklist

Emotion checklist

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
    ~~ > NOTE: Class components wrapped in withEuiTheme need to pass true as the second argument to its propUtilityForPlayground in src-docs/src/views/{component}/playground.js~

Unit tests

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

Sass/Emotion conversion process

  • Removed component from src/components/index.scss
  • Converted relevant src/amsterdam/overrides/{component}.scss styles to baseline Emotion styles
  • Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)
  • Listed var/mixin removals in changelog
    - [ ] Removed or converted component-specific Sass vars/mixins to exported JS versions - ⚠️ Not yet touching remaining $euiHeaderHeight vars or @mixin euiHeaderAffordForFixed - there's too many usages in Kibana
    - [ ] 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

- [ ] Wrapped all animations or transitions in euiCanAnimate - No CSS transitions/animations

  • Used gap property to add margin between items if using flex
  • 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)
  • SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana. (.euiHeaderLinks__list--gutterSize)

Extras/nice-to-have

  • Reduced specificity where possible (usually by reducing nesting and class name chaining)
    • Main remaining non-flat styles are .euiHeaderLinks__mobileList which it's likely not worth the effort to do so
      - [ ] 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
      - [ ] Documentation pass:

Kibana due diligence

  • 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 - No modifier class usages
  • 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
    • ✅ No removed usages of selectors within test queries
  • 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)
    • ✅ No usages, thankfully

- no instances found of `euiHeaderAlert__dismiss` anywhere in either EUI or Kibana
- remove `border-top: none` not doing anything
- last item doesn't need a border-bottom
+ remove unused/vestigal "euiLink" className
- add `shouldRenderCustomStyles` test

- remove useless tests that just test that react nodes behave like react nodes, add missing tests for props
- complex examples are borrowed heavily from docs, but might as well add them now
- the current CSS implementation of right/left is using medium behavior that doesn't account for EuiHeader's `justify-content: space-between`, and should be preferring auto margins instead (which work even when alone - see commented MDN link)

- remove `left` from being a default prop - it's not really doing anything useful and can be removed
- update downstream snapshots

- remove unnecessary `grow={false}` props in docs - grow already defaults to false
- Not doing anything and hasn't been since Amsterdam, which has been years now

+ remove usages in docs
- add `shouldRenderCustomStyles`,  improve assertions
- retain `--badge` and `--dot` modifier classes - needed by dark header styles
- convert remaining Enzyme tests to RTL
- test syntax cleanup
@elastic elastic deleted a comment from kibanamachine Jul 27, 2023
@elastic elastic deleted a comment from kibanamachine Jul 27, 2023
@elastic elastic deleted a comment from kibanamachine Jul 27, 2023
@elastic elastic deleted a comment from kibanamachine Jul 27, 2023
@cee-chen cee-chen mentioned this pull request Jul 28, 2023
@cee-chen cee-chen changed the title [Emotion] Convert EuiHeaderAlert, EuiHeaderSection, EuiHeaderSectionItem, EuiHeaderSectionItemButton, and EuiHeaderLinks [Emotion] Convert EuiHeaderAlert, EuiHeaderSection, EuiHeaderSectionItem, EuiHeaderSectionItemButton, and EuiHeaderLinks Jul 28, 2023
@elastic elastic deleted a comment from kibanamachine Jul 28, 2023
@elastic elastic deleted a comment from kibanamachine Jul 28, 2023
- EuiHeaderLinks already has a `shouldRenderCustomStyles`
- Add EuiHeaderLinks story
  - Skipping `EuiHeaderLink` story for now as it really just has one meaningful prop that's already being tested/displayed in `EuiHeaderLinks`

- Update EuiHeader story with everything put together in `sections`

- Expand EuiHeaderLogo stories
… header scss files

- `$euiHeaderHeight` Sass variables have too much usage in Kibana etc., so keeping those for now - same for header Sass mixin
@cee-chen cee-chen marked this pull request as ready for review July 28, 2023 01:34
@elastic elastic deleted a comment from kibanamachine Jul 28, 2023
@elastic elastic deleted a comment from kibanamachine Jul 28, 2023
@cee-chen cee-chen requested a review from 1Copenut July 28, 2023 01:34
@cee-chen
Copy link
Member Author

@1Copenut I think it's been a minute since I've sent an Emotion conversion your way, so giving you this one :) It has a few more components than normal in it but they're each relatively small and not super complex (or if they are complex, I at least tried to break that complexity up by commit).

LMK if you have any q's or don't have the bandwidth to review; I can ask another dev!

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

I ran through the QA and staging vs prod using a wide screen and mobile. I had a few comments of small padding differences that I wanted to cross-check. If the differences are to be expected, I'll change my comments to approved. Thanks @cee-chen !

${logicalCSS('margin-bottom', euiTheme.size.s)}
`,
euiHeaderAlert__text: css`
${euiFontSize(euiThemeContext, 's')}
Copy link
Contributor

Choose a reason for hiding this comment

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

The example in your PR build and Storybook shows a slightly smaller line-height on the text blocks than prod. 1.426 to 1.714. I think that's desirable and okay because of how the EuiFontSize helper is structured to calculate line-height. Noting it, but not flagging it as a change.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intentional across every component converted to Emotion. Caroline made those opinionated typography changes as part of the CSS-in-JS work.

src-docs/src/views/header/header_alert.tsx Show resolved Hide resolved
@@ -11,7 +11,7 @@ import {
export default () => {
return (
<EuiHeader>
<EuiHeaderSectionItem border="right">
<EuiHeaderSectionItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs examples are closer to the right edge of the section container in your PR. It looks like the gutter was not applied in the docs example. If this is by design, please disregard.

Screen Shot 2023-07-28 at 10 41 06 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

This shift to the right also appears in the fullscreen docs examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into this now!

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I really feel like the gutter sizes in the original Sass modifiers were just buggy and incorrect. gap's behavior is a far more accurate. That being said, I'll at least tweak the modifiers to double them so they behave as before (except for the extra padding against the right side of the nav - I definitely feel that falls under "bugfix".)

Copy link
Member Author

Choose a reason for hiding this comment

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

3469497 - I ended up tweaking the gap sizes to more closely match their previous production style, but not exactly (the l gutterSize in particular felt absolutely ridiculous, and there aren't any usages of that in Kibana - mostly just xs or s).

I also documented this in the changelog as an intentional change. Thanks for catching this Trevor! 🙇

@cee-chen
Copy link
Member Author

buildkite test this

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cee-chen cee-chen requested a review from 1Copenut July 31, 2023 15:16
@cee-chen
Copy link
Member Author

@1Copenut Pinging for re-review, this needs to get in before tomorrow's release

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I reviewed the changes since my last PR on Storybook in light and dark mode, small screen and wide. Took a look at the Buildkite and Jenkins previews vs. prod and compared checked out branches and hashes.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen merged commit 3e950bf into elastic:main Aug 1, 2023
1 check passed
@cee-chen cee-chen deleted the emotion/header branch August 1, 2023 19:35
cee-chen added a commit to elastic/kibana that referenced this pull request Aug 21, 2023
`v86.0.0`⏩`v87.1.0`

⚠️ The biggest set of type changes in this PR come from the breaking
change that makes `pageSize` and `pageSizeOptions` now optional props
for `EuiBasicTable.pagination`, `EuiInMemoryTable.pagination` and
`EuiDataGrid.pagination`.

This caused several other components that were cloning EUI's pagination
type to start throwing type warnings about `pageSize` being optional.
Where I came across these errors, I modified the extended types to
require `pageSize`. These types and their usages may end up changing
again in any case once the Shared UX team looks into
#56406.

---

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

- Updated the underlying library powering `EuiAutoSizer`. This primarily
affects typing around the `disableHeight` and `disableWidth` props
([#6798](elastic/eui#6798))
- Added new `EuiAutoSize`, `EuiAutoSizeHorizontal`, and
`EuiAutoSizeVertical` types to support `EuiAutoSizer`'s now-stricter
typing ([#6798](elastic/eui#6798))
- Updated `EuiDatePickerRange` to support `compressed` display
([#7058](elastic/eui#7058))
- Updated `EuiFlyoutBody` with a new `scrollableTabIndex` prop
([#7061](elastic/eui#7061))
- Added a new `panelMinWidth` prop to `EuiInputPopover`
([#7071](elastic/eui#7071))
- Added a new `inputPopoverProps` prop for `EuiRange`s and
`EuiDualRange`s with `showInput="inputWithPopover"` set
([#7082](elastic/eui#7082))

**Bug fixes**

- Fixed `EuiToolTip` overriding instead of merging its
`aria-describedby` tooltip ID with any existing `aria-describedby`s
([#7055](elastic/eui#7055))
- Fixed `EuiSuperDatePicker`'s `compressed` display
([#7058](elastic/eui#7058))
- Fixed `EuiAccordion` to remove tabbable children from sequential
keyboard navigation when the accordion is closed
([#7064](elastic/eui#7064))
- Fixed `EuiFlyout`s to accept custom `aria-describedby` IDs
([#7065](elastic/eui#7065))

**Accessibility**

- Removed the default `dialog` role and `tabIndex` from push
`EuiFlyout`s. Push flyouts, compared to overlay flyouts, require manual
accessibility management.
([#7065](elastic/eui#7065))

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

- Added beta `componentDefaults` prop to `EuiProvider`, which will allow
configuring certain default props globally. This list of components and
defaults is still under consideration.
([#6923](elastic/eui#6923))
- `EuiPortal`'s `insert` prop can now be configured globally via
`EuiProvider.componentDefaults`
([#6941](elastic/eui#6941))
- `EuiFocusTrap`'s `crossFrame` and `gapMode` props can now be
configured globally via `EuiProvider.componentDefaults`
([#6942](elastic/eui#6942))
- `EuiTablePagination`'s `itemsPerPage`, `itemsPerPageOptions`, and
`showPerPageOptions` props can now be configured globally via
`EuiProvider.componentDefaults`
([#6951](elastic/eui#6951))
- `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid` now allow
`pagination.pageSize` to be undefined. If undefined, `pageSize` defaults
to `EuiTablePagination`'s `itemsPerPage` component default.
([#6993](elastic/eui#6993))
- `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid`'s
`pagination.pageSizeOptions` will now fall back to
`EuiTablePagination`'s `itemsPerPageOptions` component default.
([#6993](elastic/eui#6993))
- Updated `EuiHeaderLinks`'s `gutterSize` spacings
([#7005](elastic/eui#7005))
- Updated `EuiHeaderAlert`'s stacking styles
([#7005](elastic/eui#7005))
- Added `toolTipProps` to `EuiListGroupItem` that allows customizing
item tooltips. ([#7018](elastic/eui#7018))
- Updated `EuiBreadcrumbs` to support breadcrumbs that toggle popovers
via `popoverContent` and `popoverProps`
([#7031](elastic/eui#7031))
- Improved the contrast ratio of disabled titles within `EuiSteps` and
`EuiStepsHorizontal` to meet WCAG AA guidelines.
([#7032](elastic/eui#7032))
- Updated `EuiSteps` and `EuiStepsHorizontal` to highlight and provide a
more clear visual indication of the current step
([#7048](elastic/eui#7048))

**Bug fixes**

- Single uses of `<EuiHeaderSectionItem side="right" />` now align right
as expected without needing a previous `side="left"` sibling.
([#7005](elastic/eui#7005))
- `EuiPageTemplate` now correctly displays `panelled={true}`
([#7044](elastic/eui#7044))

**Breaking changes**

- `EuiTablePagination`'s default `itemsPerPage` is now `10` (was
previously `50`). This can be configured through
`EuiProvider.componentDefaults`.
([#6993](elastic/eui#6993))
- `EuiTablePagination`'s default `itemsPerPageOptions` is now `[10, 25,
50]` (was previously `[10, 20, 50, 100]`). This can be configured
through `EuiProvider.componentDefaults`.
([#6993](elastic/eui#6993))
- Removed `border` prop from `EuiHeaderSectionItem` (unused since
Amsterdam theme) ([#7005](elastic/eui#7005))
- Removed `borders` object configuration from `EuiHeader.sections`
([#7005](elastic/eui#7005))

**CSS-in-JS conversions**

- Converted `EuiHeaderAlert` to Emotion; Removed unused
`.euiHeaderAlert__dismiss` CSS
([#7005](elastic/eui#7005))
- Converted `EuiHeaderSection`, `EuiHeaderSectionItem`, and
`EuiHeaderSectionItemButton` to Emotion
([#7005](elastic/eui#7005))
- Converted `EuiHeaderLinks` and `EuiHeaderLink` to Emotion; Removed
`$euiHeaderLinksGutterSizes` Sass variables
([#7005](elastic/eui#7005))
- Removed `$euiHeaderBackgroundColor` Sass variable; use
`$euiColorEmptyShade` instead
([#7005](elastic/eui#7005))
- Removed `$euiHeaderChildSize` Sass variable; use `$euiSizeXXL` instead
([#7005](elastic/eui#7005))

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Patryk Kopyciński <[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.

[Emotion] EuiHeader
4 participants