-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiThemeProvider] Add context & state required for theme CSS variables architecture #7132
[EuiThemeProvider] Add context & state required for theme CSS variables architecture #7132
Conversation
@1Copenut As a heads up, I'll be assigning more PRs to you this week as Bree is out and since you're already on support this week. |
// Our current version of jsdom doesn't yet support :root (currently on v11, | ||
// need to be on at least v20), so we'll mock something to assert on in the interim |
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.
Reference: https://github.com/jsdom/jsdom/releases/tag/20.0.0
We'll need to update Jest to latest to get a more recent version of jsdom (#6813)
d82b52d
to
81ebd19
Compare
src/services/theme/context.ts
Outdated
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.
[attaching to a semi-random file for threading]
I'm debating making this a hook, e.g. useEuiCSSVariables()
. Right now an EUI component will have to do this:
import React, { useContext } from 'react';
import { EuiNestedThemeContext } from '../services/theme';
const EuiComponent = () => {
const { setGlobalCSSVariables } = useContext(EuiNestedThemeContext);
}
which feels like it could be cleaner/more elegant 🤔
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.
I'm also debating adding a section about this in our Emotion wiki. I'm not totally sure what to write just yet and that may become clearer once I actually have the EuiHeader work in front of me, so I may hold off on that wiki doc until the last PR that actually merges in the feature branch into main.
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.
I added a hook after all in 6f4f6b2, but will continue to hold off on a wiki document for a bit
export const WrapperCloneElement: Story = { | ||
render: () => ( | ||
<> | ||
<EuiThemeProvider wrapperProps={{ cloneElement: true }}> | ||
<main className="clonedExample"> | ||
This example should only have 1 main wrapper rendered. | ||
</main> | ||
</EuiThemeProvider> | ||
</> | ||
), | ||
}; |
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.
This story isn't related to this PR, I just thought I'd add it as a tech debt item while here (and also as a comparison to the existing stories)
Preview documentation changes for this PR: https://eui.elastic.co/pr_7132/ |
💚 Build Succeeded
|
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.
I ran through the Storybook examples and updated a couple on the fly to see changes on my local. Having this ability and the tests helped document the changeset and understand how the CSS variables flow up.
Thanks Trevor! Going to go ahead and merge this in, but if anyone on the team has any other feedback, this is a feature branch and I'm happy to continue tweaking functionality and architecture! |
Summary
This PR adds the architecture for CSS variables to be set by EUI components. This is part of the work needed for #7025, in particular:
Screencaps
Global theme variables:
Local theme variables:
QA
gh pr checkout 7132
yarn storybook
:root
.euiThemeProvider
wrapper:root
General checklist
and cypress tests[ ] Added documentationNo documentation, this architecture is meant for EUI internal components, not for consumers[ ] A changelog entry exists and is marked appropriatelyNo changelog, see above reasoning