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

[EuiThemeProvider] Add context & state required for theme CSS variables architecture #7132

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Aug 28, 2023

Summary

⚠️ NOTE: This PR is merging into a feature branch, as it doesn't make much sense on its own without another component utilizing its behavior.

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:

Some components have style-related variables that may need to be reused by consumers, or even by other components within EUI. Examples of these are euiFormVariables and euiHeaderVariables.
Instead of moving these style variables outside our .styles.ts files and continuing to export them publicly, @tkajtoch has suggested switching them to CSS variables instead. This will involve:
[...] Declaring these component style vars once at the top level in our global styles (ensuring consumers have access to the default vars everywhere in the app)

Screencaps

Global theme variables:
global root variables

Local theme variables:
theme variables

QA

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox - caniuse CSS variables
  • Added or updated jest and cypress tests
  • [ ] Added documentation No documentation, this architecture is meant for EUI internal components, not for consumers
  • [ ] A changelog entry exists and is marked appropriately No changelog, see above reasoning

@cee-chen cee-chen changed the title [EuiThemeProvider] Add context/state required for theme CSS variables architecture [EuiThemeProvider] Add context & state required for theme CSS variables architecture Aug 28, 2023
@cee-chen cee-chen requested a review from a team August 28, 2023 17:45
@cee-chen cee-chen marked this pull request as ready for review August 28, 2023 17:45
@cee-chen cee-chen requested a review from 1Copenut August 28, 2023 17:45
@cee-chen
Copy link
Member Author

@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.

Comment on lines +159 to +160
// 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
Copy link
Member Author

@cee-chen cee-chen Aug 28, 2023

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)

Copy link
Member Author

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 🤔

Copy link
Member Author

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.

Copy link
Member Author

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

Comment on lines +23 to +33
export const WrapperCloneElement: Story = {
render: () => (
<>
<EuiThemeProvider wrapperProps={{ cloneElement: true }}>
<main className="clonedExample">
This example should only have 1 main wrapper rendered.
</main>
</EuiThemeProvider>
</>
),
};
Copy link
Member Author

@cee-chen cee-chen Aug 28, 2023

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)

@kibanamachine
Copy link

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

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 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.

@cee-chen
Copy link
Member Author

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!

@cee-chen cee-chen merged commit d2fec47 into elastic:theme-css-variables Aug 28, 2023
6 checks passed
@cee-chen cee-chen deleted the eui-css-variables branch August 28, 2023 23:22
cee-chen added a commit that referenced this pull request Aug 30, 2023
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.

4 participants