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] [Breaking] Deprecating EUI theme to Legacy #5222

Merged
merged 4 commits into from
Oct 1, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 28, 2021

Promotes Amsterdam theme as “default”

Most of the changes in this doc were from renaming the Sass files under src/themes to remove the eui prefix and rename the default to legacy.

  1. src/themes/eui --> src/themes/legacy
  2. src/themes/eui-amsterdam --> src/themes/amsterdam

It also adjusts the Sass pipeline from compiling the legacy theme as the default CSS to the amsterdam theme.
This is a breaking change as it changes what theme is now rendered when importing the CSS.

  1. src/themes/legacy/legacy_[colorMode].scss --> eui_legacy_[colorMode].css
  2. src/themes/amsterdam/theme_[colorMode].scss --> eui_theme_[colorMode].css

For a high level view of this pipeline change, see this Whimsical: https://whimsical.com/how-to-consume-eui-a-history-YSw3zLABW6rKZzWxMBeJyE@2Ux7TurymQ2CEaW5dZV4

I also moved any exported constants or utilities that are theme-specific like the theme keys to their respective src/themes/[theme] folder.

Docs

This changes the theme selector from saying "Amsterdam" to changing the old theme to say "Legacy" and adds an EuiTour (with dismissed state in local storage) to point this out to users.
Screen Shot 2021-09-29 at 14 15 18 PM

TODO:

  • [ ] Update the Getting Started docs: I have this mostly done already but will apply it in another follow-up PR

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
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest 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 Feature branch

Promotes Amsterdam theme as “default”
@kibanamachine
Copy link

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

Copy link
Contributor Author

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

I've outlined the high level changes in the PR summary but I'll breakdown my changes in the following comments.

theme={
theme.includes('amsterdam') ? EuiThemeAmsterdam : EuiThemeDefault
}
theme={EUI_THEMES.find((t) => t.value === theme)?.provider}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a provider key to the EUI_THEME type that will be the actual themes like EuiThemeAmsterdam to be more scalable than a simple ternary.

Comment on lines 19 to +20
case 'dark':
palette = darkColors;
palette = { ...lightColors, ...darkColors, ...darkAmsterdamColors };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also fixes the crashing of the Colors Guidelines page in master in dark mode.

Comment on lines +74 to +75
// Packages
@import '../../packages/index';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this from the basic manifest to this component list because it technically is one.

src/components/tour/_tour.scss Outdated Show resolved Hide resolved
src/components/tour/tour_step.tsx Outdated Show resolved Hide resolved
@@ -150,7 +150,6 @@ export {
getOn,
mergeDeep,
setOn,
isDefaultTheme,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this utility to src/themes/legacy because it is specific to that theme.

Comment on lines -28 to -29
DEFAULT_NAME_KEY,
AMSTERDAM_NAME_KEY,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same with the naming keys.

@cchaos cchaos marked this pull request as ready for review September 29, 2021 18:40
@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor

Looks like some duplicate changes in #5225. Do you want to remove them from this branch?

@cchaos
Copy link
Contributor Author

cchaos commented Sep 30, 2021

I will when that merges. Right now, they cause TS errors.

…js/breaking-legacy

# Conflicts:
#	src/components/tour/_tour.scss
#	src/components/tour/tour_step.tsx
@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM!
Let's get this in before the reset/global PR; I think any conflicts will be easier to resolve in that branch.

@cchaos cchaos merged commit f00cacc into elastic:css-in-js/breaking Oct 1, 2021
@cchaos cchaos deleted the css-in-js/breaking-legacy branch October 1, 2021 19:32
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