[EuiHeader] Set up CSS variable for fixed header affordance; [Emotion] Set up CSS variable architecture #7151
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7151/ |
|
@tkajtoch has expressed an interest in reviewing this feature before it merges in, so going to hold for his thoughts before merge! |
| @@ -126,14 +126,14 @@ const euiOverflowShadowStyles = ( | |||
| * Others like Safari, won't show anything at all. | |||
| */ | |||
| interface _EuiYScroll { | |||
| height?: CSSProperties['height']; | |||
| height?: CSSProperties['height'] | false; | |||
There was a problem hiding this comment.
I'm not sure if I like having false as the magic value here. null isn't better, undefined defaults to 100% (but so does 0 which may not be expected). What about inherit though? It's a real CSS value and it means almost exactly what our implementation does.
There was a problem hiding this comment.
Very fair! I reverted the euiYScroll change in 4f08b9a, and simply passed the height calc directly to the function (not sure why that didn't occur to me as opposed to overriding via CSS haha).
edit: I pushed up too early, you were also totally right that height: inherit works perfectly and saves us a calc()! 🎉 I've updated both the collapsible nav and flyout styles accordingly: d2e834c
| </EuiColorModeContext.Provider> | ||
| <> | ||
| {isGlobalTheme && themeCSSVariables && ( | ||
| <Global styles={{ ':root': themeCSSVariables }} /> |
There was a problem hiding this comment.
:root won't work inside Shadow DOM because :root pseudoclass targets Document (<html>, <svg> tags) and not DocumentFragment that Shadow DOM uses. We should probably do one of two things if we want to address it in this PR:
- Add
shadowDom: booleanprop and use:hostinstead of:rootwhen true - Automatically detect if EUI is used inside Shadow DOM by
node.getRootNode() instanceof ShadowRoot(would require getting a ref)
There was a problem hiding this comment.
Thanks a million for catching this Tomasz! Just curious, could we simply target both via something like this:
:root,
:host {
/* ... CSS variables */
}And let CSS determine for us if :host/:root does or doesn't apply?
There was a problem hiding this comment.
Disregard the above, it breaks a lot and CSS gets very confused 😅 Disregard this, it's Emotion that's confused
There was a problem hiding this comment.
Alright, so even if I change the the selector to just :host, this currently won't work as-is with Shadow DOM because Emotion is still injecting the styles into the global <head> as opposed to the shadow container/host. Either way our current Emotion setup doesn't work in shadow DOM, period, and will need a lot of adjustment (at both the theme, provider, and Emotion level) to account for this.
@tkajtoch I'm going to move ahead with merging this PR as-is since full working shadow DOM support is going to be its own effort, and I don't think we should block the majority of current consumers benefiting from this work for a small subsection of consumers (when shadow DOM currently doesn't work in prod in any case).
cee-chen
force-pushed
the
theme-css-variables
branch
from
da1e557
to
d2e834c
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7151/ |
💚 Build Succeeded
History
|
Summary
This is a feature branch merge. Please see the below constituent PRs for more information:
--euiFixedHeadersOffsetCSS variable #7144QA
General checklist
and cypress tests[ ] A changelog entry exists and is marked appropriatelySkipping for feature branch merge, changelogs for individual PRs were included