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] Support extensions via overrides prop #4547

Merged

Conversation

thompsongl
Copy link
Contributor

Summary

Allows for extending a theme object with custom properties (including top-level sections).

Custom properties will be computed 1) after all "standard" theme values have been computed and 2) in the order they are defined. This means that they will have access to all computed values and can have computational dependencies just like standard theme properties.

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

@thompsongl
Copy link
Contributor Author

@chandlerprall @cchaos
Before I mark as "Ready for review", I want to consider the name of the overrides prop.
Now that it doesn't consist solely of overrides, is there something more appropriate? extensions comes to mind. Also, apply as more generic term. Thoughts or ideas?

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

chandlerprall commented Feb 18, 2021

I think extensions is the better fit. Even if it is a single extension overriding an already-defined value, that represents a re-definition of the theme.

Similarly, in an eslint config you can specify extends: ['airbnb'] and then overwrite one of its settings.

This means that they will have access to all computed values and can have computational dependencies just like standard theme properties.

🎉 🎉 🎉

@cchaos
Copy link
Contributor

cchaos commented Feb 18, 2021

My only, completely biased opinion, is that extensions is harder to remember and write than overrides. extends is actually not bad as a term like the eslint example, but the tense doesn't work for us.

Also I think that extends is quite the opposite. It accepts a while config as a starting point, where as the starting point already exists as the current theme and the consumer is adding/overriding it.

Thinking 🤔

@kibanamachine
Copy link

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

@thompsongl thompsongl marked this pull request as ready for review February 18, 2021 23:10
@thompsongl thompsongl mentioned this pull request Feb 24, 2021
7 tasks
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.

Just a couple questions and clarifications.

Comment on lines +201 to +203
euiColorPrimary: '#F56407',
myColor: computed(['colors.euiColorPrimary'], ([primary]) => primary),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just spitting out the same value as in : myColor = #F56407?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Just testing that computed values worked here.

src/services/theme/README.md Outdated Show resolved Hide resolved
src/services/theme/README.md Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

LGTM, let's get this in so we can also update the docs PR

@thompsongl
Copy link
Contributor Author

jenkins test this

flaky tooltip

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Suggested a rename but otherwise this LGTM!

src/components/common.ts Outdated Show resolved Hide resolved
Co-authored-by: Chandler Prall <[email protected]>
@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

jenkins test this

flaky context_menu

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit 90740eb into elastic:feature/css-in-js Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants