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

[CSS-in-JS] Add provider to generated codesandbox examples #5253

Merged

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Oct 7, 2021

Summary

Adds a global provider to generated codesandbox examples.

Note that EuiThemeProvider is currently used because EuiProvider is not yet in a public release. Once we confirm the output is correct, I will replace all instances with EuiProvider.
Also note that we cannot guarantee that static and global style source order is correct. Will need to wait until EuiProvider is in a public release and adjust as necessary.

Checklist

  • Check against all themes for compatibility in both light and dark modes

~- [ ] Checked in mobile~~
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation

- [ ] Added or updated jest and cypress tests
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] A changelog entry exists and is marked appropriately

  • Replaced EuiThemeProvider with EuiProvider

@thompsongl
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link

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

@thompsongl thompsongl requested a review from cchaos October 7, 2021 20:31
@thompsongl thompsongl marked this pull request as ready for review October 7, 2021 20:31
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Oct 7, 2021

I know that this isn't really testable on CodeSandbox link so I'll just ask here:

  1. Just the theme provider doesn't add the reset file right?
  2. Just like the imported .css, is it possible to apply the provider only when not using the legacy theme?

@thompsongl
Copy link
Contributor Author

Just the theme provider doesn't add the reset file right?

Correct, but EuiProvider will add the reset styles

Just like the imported .css, is it possible to apply the provider only when not using the legacy theme?

I have it passing theme={null} when using the legacy theme, but I could omit EuiProvider entirely for that case

@cchaos
Copy link
Contributor

cchaos commented Oct 11, 2021

Correct, but EuiProvider will add the reset styles

Then I suggest just going ahead with EuiProvider now even though the import itself won't work until release.

@cchaos
Copy link
Contributor

cchaos commented Oct 11, 2021

I'd like the setup to mimic our own as much as possible.

@thompsongl
Copy link
Contributor Author

Then I suggest just going ahead with EuiProvider now even though the import itself won't work until release.

Yes, the plan is to do so in this PR.

I'd like the setup to mimic our own as much as possible.

In the docs, we'll include EuiProvider even if the legacy theme is selected. In that sense including it the codesandbox link sounds correct. And as soon as we start converting components, EuiProvider or EuiThemeProvider will be required for functionality

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Oct 11, 2021

In the docs, we'll include EuiProvider even if the legacy theme is selected.

Oh sorry for the confusion here. I think what I meant more was providing what is required depending on the theme. Since EuiProvider does nothing currently if theme = null, then we should require it in the Codesandbox link. I honestly think seeing EuiProvider theme={null} is less helpful than just leaving it out completely.

And as soon as we start converting components, EuiProvider or EuiThemeProvider will be required for functionality

I currently think this may be questionable. My thought would be that the legacy theme would still work with just straight compiled .css. But if it turns out they are required, I think we can adjust the link then.

So to summarize, for this PR I think we should do the following:

If current/Amsterdam theme

<EuiProvider>
  <Demo />
</EuiProvider>

If legacy theme

<Demo />

@thompsongl
Copy link
Contributor Author

thompsongl commented Oct 11, 2021

I'm good omitting the provider for now.
To clarify my "required" comment: when we start converting components to use Emotion, anything that uses useEuiTheme will need EuiThemeProvider in the app. So Sass is still in option, but there are some technical requirements for converted components when it comes to the theme provider setup.

Edit: We might be able to bypass Emotion styles if EuiThemeProvider is absent (and therefore useEuiTheme returns a null theme`). Unverified at this point, but we can explore.

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool beans. LGTM

@thompsongl thompsongl merged commit f8d8675 into elastic:css-in-js/breaking Oct 14, 2021
thompsongl added a commit that referenced this pull request Nov 30, 2021
* emotion as peerDep; babel and typescript

* jest config

* isDefaultTheme util

* CL TODO

* update snapshots

* [CSS-in-JS] [Breaking] Deprecating EUI theme to Legacy (#5222)

* [CSS-in-JS] Global style reset with `emotion` (#5122)

* global reset with emotion

* amsterdam overrides

* text-size-adjust

* ams reset

* invert ::selection

* Renamed `isDefaultTheme` utility to `isLegacyTheme`

Default will mean something different when Amsterdam becomes default so I want to head this off now by checking for “Legacy”

* Quick cleanup

* Fixed up scrollbar function

* Removed `useEuiFont` in favor of a single reset on the elements that need it

* Added `body` key to global `font` for setting base body font settings at the global level

* Some more fixes based on new diff

* global reset with emotion

* amsterdam overrides

* text-size-adjust

* ams reset

* invert ::selection

* Renamed `isDefaultTheme` utility to `isLegacyTheme`

Default will mean something different when Amsterdam becomes default so I want to head this off now by checking for “Legacy”

* Quick cleanup

* Fixed up scrollbar function

* Removed `useEuiFont` in favor of a single reset on the elements that need it

* Added `body` key to global `font` for setting base body font settings at the global level

* Some more fixes based on new diff

* Pulling our reset from global styles

* SVG `hacks` directly in reset file

* Fixed reset import

* euiprovider; reset styles

* use euiprovider with emotion/cache in docs

* Decision clean up

* Fixing a few things for legacy dark mode based on not having a provider

* Revert "Fixing a few things for legacy dark mode based on not having a provider"

This reverts commit 127f12e.

* Actually keeping `colorMode` even if `theme = null`

* Fixing merge

* Moved reset sass folder to `legacy`

* fix wiki

* euiprovider docs

* fix provider docs

* remove commented import

Co-authored-by: cchaos <caroline.horn@elastic.co>

* [CSS-in-JS] Add provider to generated codesandbox examples (#5253)

* add provider to generated codesandbox examples

* indentation

* euithemeprovider -> euiprovider

* legacy theme

* fix snapshot tests

* [CSS-in-JS] Provider for fullscreen examples; Context refactor (#5309)

* refactor to extract context component

* refactor for fewer updates on route changes

* refactor AppView to function component

* [CSS-in-JS] [Breaking] Better Getting Started page (#5299)

* Moved all color Sass files to their respective theme folders
- Duplicates variables, but makes it actually capable of customization
- New files for code & vis colors that weren’t theme specific
* Remove outdated theming wiki page
* Cleanup (in MD files too)

* fix heading level

* update changelog page regex to fully remove the master section

* manual merge

* update release script changelog regex

* Cleanup some docs

* Hide LanguageSelector tour on Getting Started page

* Some GS page cleanup

* PR feedback

* cl

* Apply suggestions from code review

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* lint fix

* make amsterdam default

* provider updates

* fix cypress styles

* PR feedback:

- Removed blank mixins file
- Added `yarn add` block for dependencies
- Added props table to bottom of Provider docs page

* move keydown inside useeffect

* Reusing correct Sass customization snippet under Customizing

* fix cypress path

* update export

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
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.

3 participants