Upgrade EUI to v88.2.0 #165790
Upgrade EUI to v88.2.0 #165790
Conversation
cee-chen
force-pushed
the
eui-88.2.x
branch
4 times, most recently
from
755f4ab
to
c92d139
Compare
…extraAction` to example custom CSS
- likely not important to preserve the text-decoration CSS here
- include global banner if present, account for chrome visible or not + flatten conditional modifiers, to make overriding by other usages easier + remove offsets logic in _banner.scss, easier to handle it all in one place note: all flyouts/collapsible navs should inherit automatically from the `--euiFixedHeadersOffset` var
+ delete unused `kibanaFullBodyMinHeight` mixin
- by obtaining the height value from the DOM/CSS var
- Remove deprecated EUI mixin (now handled by CSS below) - Remove selector-specific overrides; update their usages to use the global CSS variable instead
- remove manual offset calculation and fall back to EuiPageSidebar's default inline styles instead, which automatically uses `--euiFixedHeadersOffset` + bonus: fix broken `:hover` effect on solution side navs by restoring missing className + continue dismantling `kbnAffordForHeader` mixin
+ remove `kbnStickyMenu` entirely - it looks like old code and it's not clear to me exactly how that behavior is being used, or if it should be using EuiPageSidebar intead - will ping Fleet team to ask
- # of fixed EuiHeaders should now be automatically detected and handled by EUI without needing any custom body logic
+ some snapshot updates around the new header CSS var
- was relying on the old selectors to set the correct height - use the new EUI var instead and remove direct className reference
- className no longer has any direct styles applied, and as far as I can tell doesn't need any associated header-related flyout logic
| @@ -138,7 +138,7 @@ export default function ({ getPageObjects, getService, updateBaselines }) { | |||
| 'web_logs_map', | |||
| updateBaselines | |||
| ); | |||
| expect(percentDifference).to.be.lessThan(0.02); | |||
There was a problem hiding this comment.
Can you add some context for increasing this threshold. Maybe some screen shots from each or a diff screen shot. A 50% increase is pretty big.
Maybe the baselines need to be updated instead?
There was a problem hiding this comment.
To be honest, I've struggled with Kibana's visual snapshot tests in the past. When I've previously tried to update the baselines locally, the new images appeared to be using a totally different set of DPI/dimensions than CI and it ended up failing completely on CI.
I'd appreciate either being able to pair with someone on your team for that, or alternatively having your team update baselines in a follow-up PR after this one. Please let me know what you prefer!
There was a problem hiding this comment.
or alternatively having your team update baselines in a follow-up PR after this one. Please let me know what you prefer!
We can just make a PR to update the snapshots afterwards. That will be easier for you
There was a problem hiding this comment.
Thanks a million @nreese!
There was a problem hiding this comment.
Thanks for the upgrade @cee-chen This is working great. I tested it out and I found a few places within security_solution and fleet where the var() function's fallback values need to be adjusted to account for when euiFixedHeadersOffset is not available.
I've not tested the other plugins but they may need this adjustment as well.
packages/core/chrome/core-chrome-browser-internal/src/ui/project/app_menu.tsx
Show resolved
Hide resolved
...olicy/create_package_policy_page/components/steps/components/package_policy_input_stream.tsx
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/timelines/components/flyout/pane/index.tsx
Show resolved
Hide resolved
| 100vh - ${(props) => parseFloat(props.theme.eui.euiHeaderHeightCompensation) * 2}px | ||
| ); | ||
| // Set the min height to the viewport size minus the height of any global Kibana headers | ||
| min-height: calc(100vh - var(--euiFixedHeadersOffset)); |
There was a problem hiding this comment.
This breaks the header spacing if euiFixedHeadersOffset is not available. Also I've not been able to figure this out yet. It doesn't work with a fallback of 96px. You might want to restore the min-height value that uses eui props for now. @juliaElastic ☝🏼
There was a problem hiding this comment.
Thanks for checking, I'll restore it. At some point the old styled-components props will be deprecated/going away, but right now they're frozen during the Emotion conversion and aren't at risk for removal for another 3+ months.
There was a problem hiding this comment.
Actually, @ashokaditya do you mind explaining how exactly you're getting the state of "euiFixedHeadersOffset is not available"?
The only scenario that should happen in production is when no fixed EuiHeaders are on the page, in which case falling back to 0 should be correct for all var() usages. If you're testing by unchecking the --euiFixedHeadersOffset variable definition in browser devtools, that would not be a realistic scenario end-users face.
There was a problem hiding this comment.
Just to close the loop on this comment and the above comments - Ash was unchecking the definition in devtools, which isn't something that will happen in production. I've marked the above code suggestions as resolved and fixed all var() usages like this one to have 0 fallbacks in b530f22 🎉
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
## Summary [Latest upgrade of EUI](elastic#165790) removed the `isSticky` property that we were using in Integrations sidebar. This PR is adding it back with some css properties. https://github.com/elastic/kibana/assets/16084106/ffe0e1ed-cf70-45b7-8d8d-cb0c5f53e843
Major changes in this PR:
Removal of
.euiAccordionFormclassNamesEUI is moving away from providing global
classNamesstyles for components - where possible, we want to provide props as opposed to styles. As part of our ongoing Emotion conversion, we have removed the followingEuiAccordion-specific classes:.euiAccordionForm(replaced withborders="horizontal").euiAccordionForm__button(replaced withbuttonProps={{ paddingSize: 'm' }}).euiAccordionForm__titlestyles - this was only removing text underlines on hover. If still desired, re-add this behavior with custom CSS..euiAccordionForm__extraAction- there was 1 usage of this in Kibana in Watcher, which was converted to one-off custom inline Emotion CSS instead.This change accounts for the first 3-4 commits in the PR.⚠️ If your team was one of the 4-5 teams affected by this change, we would greatly appreciate your help QAing the UI and ensuring it looks as desired/the same as before.
Fixed
EuiHeaderaffordanceThe Sass
euiHeaderAffordForFixedmixin has been deprecated and replaced by a global--euiFixedHeadersOffsetCSS variable. This variable updates independently and dynamically based on the number of fixed headers on the page, and is usable by both Sass and Emotion. All underlying EUI components that need to account for fixed headers (such as flyouts and page sidebars/templates) have been updated to consume this new variable.This change cleans up a great deal of Sass code, and is incidentally extremely timely with serverless efforts (as serverless has only one fixed header and the classic Kibana chrome has two).
This change constitutes a majority of the commits in this PR, which involve removing various fixed Sass header variables and replacing them with the new CSS variable. I strongly recommend reviewing changes by commit if possible to see any associated changes that make sense together with any of your touched file(s).⚠️ If your team was affected by this change (primarily due to custom header layouts), your help would be greatly appreciated QAing your app to ensure no UI regressions in that regard have occurred.
v88.1.0⏩v88.2.088.2.0EuiTextTruncatecomponent, which provides custom truncation options beyond native CSS (#7116)EuiHeaders now set a global CSS--euiFixedHeadersOffsetvariable, which updates dynamically based on the number of fixed headers on the page. (#7144)EuiFlyouts now dynamically set their position, height, and mask based on the number of fixed headers on the page. (#7144)EuiPageSidebars now dynamically set their position and height based on the number of fixed headers on the page. This can still be overridden via thesticky.offsetprop if needed. (#7144)EuiPageTemplatenow dynamically offsets content from any fixed headers on the page. This can still be overridden via theoffsetprop if needed. (#7144)EuiAccordionwith a newbordersprop (#7154)EuiAccordionwith a newbuttonProps.paddingSizeprop (#7154)Deprecations
euiHeaderAffordForFixedmixin. Use the new global CSSvar(--euiFixedHeadersOffset)variable instead. (#7144)CSS-in-JS conversions
classNamesthat are component-specific. As part of this effort, we have removed the followingEuiAccordion-specific classes: (#7154).euiAccordionFormstyles. Use theborders="horizontal"prop instead.euiAccordionForm__buttonstyles. Use thebuttonProps={{ paddingSize: 'm' }}prop instead.euiAccordionForm__extraActionstyles. Convert this to your own custom CSS if necessary..euiAccordionForm__titlestyles. Convert this to your own custom CSS if necessary.