-
Notifications
You must be signed in to change notification settings - Fork 842
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 EuiHeader and EuiHeaderLogo #6878
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6878/ |
- makes it easier to work with and the distinction isn't necessary at this point
- see `src/global_styling/variables/_header.scss` for variable definitions - NOTE: not removing the Sass variables for now as they're used by other Sass files and also by Kibana
ba467d0
to
21ad29b
Compare
<EuiHeader> | ||
<EuiHeaderLogo {...args} /> | ||
</EuiHeader> |
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.
Quick note - this will look a little odd/non-centered right now in Storybook. This is because the EuiHeaderSectionItem
styles are not yet in Emotion / not yet in Storybook.
// Layout vars | ||
$euiHeaderHeight: $euiSizeXXL + $euiSizeS !default; | ||
$euiHeaderChildSize: $euiHeaderHeight !default; | ||
$euiHeaderChildSize: $euiSizeXXL !default; | ||
|
||
// Use the following variable in other components to afford for the fixed header | ||
$euiHeaderHeightCompensation: $euiHeaderHeight + 1px !default; | ||
$euiHeaderHeightCompensation: $euiHeaderHeight !default; |
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.
We can likely delete the first two variables once all header components are converted to Emotion, but $euiHeaderHeightCompensation
is used in multiple places in Kibana and will need to be maintained:
https://github.com/search?q=repo%3Aelastic%2Fkibana%20euiHeaderHeight&type=code
@breehall no rush on this PR, I definitely want it getting in after this week's release to limit the # of Emotion conversions in this week's Kibana upgrade. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6878/ |
- remove modifier map + remove `border-top` override - unnecessary since the header already has a `bottom-border`, and unused since I don't see this pattern anywhere in EUI or Kibana + fix incorrect logical mapping - horizontal is `inline` and vertical is `block` 🤦
- Only used in `src-docs/` and can be totally replaced by `panelPaddingSize` 🤷 - not used anywhere in Kibana
- Merge Amsterdam overrides and default CSS - Remove unused `@include euiLink` mixin - it's basically not doing anything whatsoever since the hover/focus states are being unset later - remove unused icon size override - XL icons aren't being used anyway and the modifier map was removed in the Emotion conversion
+ opinionated change - tweak padding-left on logo text to match other padding on `xs` breakpoint + remove unused `vertical-align: middle` CSS - flex handles alignment instead
- convert to RTL from Enyzme + add a test for `euiHeaderLogo__text`
- to match other subcomponents
Preview documentation changes for this PR: https://eui.elastic.co/pr_6878/ |
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.
Apologies in my delay on getting this PR reviewed! I went through your checklist of items and each example looks good.
✅ EuiHeader: test that the position and theme props work as expected
✅ EuiHeaderLogo: test that the iconType and iconTitle props work as expected
These look and function as they should. I also ran through controls on keyboard and I didn't notice any issues.
✅ Go to https://eui.elastic.co/pr_6878/#/layout/flyout and confirm all flyouts do not collide with the header
Each flyout looks good in mobile and desktop. Nothing collides or overlaps.
✅ Go to https://eui.elastic.co/pr_6878/#/layout/header/elastic-pattern and confirm the collapsible nav flyout does not collide with the header
"horizontal": "inset-inline", | ||
"vertical": "inset-block", |
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.
Did a quick check and it doesn't look any other styles are currently using ${logicalCSS('horizontal')}
or ${logicalCSS('vertical')}
. So there shouldn't be any regressions to worry about with this!
className="euiHeaderProfile" | ||
responsive={false} | ||
> | ||
<div style={{ width: 300 }}> |
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.
Was the change from width: 320
to width: 300
intentional? I see it in header/header_alert.tsx
as well but just wanted to be sure.
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.
Yes, this was intentional and was caused by the removed .euiHeaderProfile
className below. That class was setting some padding, but could have been replaced by panelPaddingSize
without requiring custom CSS. Doing so means there's extra width now however, so I reduced it slightly to a nice round number.
The width honestly doesn't really matter and is just an implementation detail/example - it also only affects this specific example and not consumer usage.
Preview documentation changes for this PR: https://eui.elastic.co/pr_6878/ |
`eui@83.0.0` ⏩ `83.1.0` --- ## [`83.1.0`](https://github.com/elastic/eui/tree/v83.1.0) - Added `placeholder` prop to `EuiInlineEdit` ([#6883](elastic/eui#6883)) - Added `sparkles` glyph to `EuiIcon` ([#6898](elastic/eui#6898)) **Bug fixes** - Fixed Safari-only bug for single-line row `EuiDataGrid`s, where cell actions on hover would overlap instead of pushing content to the left ([#6881](elastic/eui#6881)) - Fixed `EuiButton` not correctly merging in passed `className`s with its base `.euiButton` class ([#6887](elastic/eui#6887)) - Fixed `EuiIcon` not correctly passing the `style` prop custom `img` icons ([#6888](elastic/eui#6888)) - Fixed multiple components with child props (e.g. `buttonProps`, `iconProps`, etc.) unsetting EUI's Emotion styling if custom `css` was passed to the child props object ([#6896](elastic/eui#6896)) **CSS-in-JS conversions** - Converted `EuiHeader` and `EuiHeaderLogo` to Emotion ([#6878](elastic/eui#6878)) - Removed Sass variables `$euiHeaderDarkBackgroundColor`, `$euiHeaderBorderColor`, and `$euiHeaderBreadcrumbColor` ([#6878](elastic/eui#6878)) - Removed Sass mixin `@euiHeaderDarkTheme` ([#6878](elastic/eui#6878))
Summary
EuiHeader
to EmotioneuiHeaderVariables()
util (similar toeuiFormVariables()
) for other Emotion components to grab shared variables in the future (mostly height related).euiHeader--
modifier classes$euiHeader
Sass variables that are not used by either EUI or Kibana@euiHeaderDarkTheme
Sass mixin and replaces it with an Emotion util (internal only). Also convertsEuiSelectableTemplateSitewide
CSS that was specific to the header dark theme to be nested within the dark theme.euiHeaderProfile
Sass - appears to only be used in docs and not in actual src codeEuiHeaderLogo
to Emotion and moves all related files to a newheader/header_logo
subdirectoryI strongly recommend following along by commit.
QA
gh pr checkout 6878
&&yarn storybook
position
andtheme
props work as expectediconType
andiconTitle
props work as expectedGeneral checklist
Emotion checklist
Kibana usage
{euiComponent}-
(case sensitive) to check for usage of modifier classes- [ ] If usage exists, consider converting to a- no usagesdata
attribute so that consumers still have something to hook intoGeneral
className(s)
read as expected in snapshots and browsers- [ ] Checked component playground- No playground~~ > NOTE: Class components wrapped in
withEuiTheme
need to passtrue
as the second argument to itspropUtilityForPlayground
insrc-docs/src/views/{component}/playground.js
~Unit tests
shouldRenderCustomStyles()
test was added and passes with parent component and any nestedchildProps
(e.g.tooltipProps
)- [ ] Removed anymount()
ed snapshots in favor ofrender()
or a more specific assertionSass/Emotion conversion process
$euiSize
toeuiTheme.size.base
)- [ ] Ranyarn compile-scss
to update var/mixin JSON files- [ ] Simplifiedcalc()
tomathWithUnits
if possible (if mixing different unit types, this may not be possible)- [ ] Added an@warn
deprecation message within theglobal_styling/mixins/{component}.scss
file- [ ] Removed component fromNot done with full component conversion yetsrc/components/index.scss
src/amsterdam/overrides/{component}.scss
styles to baseline Emotion stylesCSS tech debt
- [ ] Wrapped all animations or transitions ineuiCanAnimate
- [ ] Used- no margins, although possibly padding might be convertible - will determine in next PRgap
property to add margin between items if using flex-inline
and-block
logical properties (check inline styles as well as CSS)DOM Cleanup
euiComponent
,euiComponent__child
)Kibana due diligence
**/target, **/*.snap, **/*.storyshot
for less noise) foreui{Component}
(case sensitive) to find:.euiHeader
usageeuiBadge
class on a div instead of simply using theEuiBadge
component)Extras/nice-to-have
- [ ] 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: