-
Notifications
You must be signed in to change notification settings - Fork 21
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
chore: add new base palette colors & use them in pentaho+ theme #4188
Conversation
@@ -135,12 +135,10 @@ const categorical = { | |||
const common = { | |||
...base, | |||
...categorical, | |||
// TODO: add "base palette" colors? |
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.
If we spread the "base palette" colors here, they'll be injected as CSS variables and available in theme.colors.*
, but these tokens do not vary depending on theme.
- Should we inject the CSS vars?
- Should we make them available only on the "runtime"
theme
object (fromuseTheme
)? - Should we keep it as a separate static
import
?
1.
seems wasteful, and if we don't want to promote the usage of the palette colors directly 3.
might make the most sense
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.
The "base palette" are the colors added to colosPalette.ts, right?
Doing only 2
means that users will only be able to use these colors inside a function component or hook (otherwise they'll need to "hack" their way into using them somewhere else). I'm not 100% comfortable with that.
We could do 3
but it seems a bit confusing to me (and to users) to import colors from two different places: theme
and static imports.
I agree that 1
seems wasteful, and we would be polluting the stylesheets with unnecessary CSS vars. However, it makes sense to have access to these colors when using theme
. Can't we do a mix of both inside the theme
object? Some colors are CSS vars and other ones are static values? For example we have utils like spacing
or alpha
that are static.
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.
Yeah, we could optimize which colors are injected as CSS vars 👍
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.
Tokens are available from theme.palette
(alternative suggestions welcome)
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.
LGTM 🚀
Some questions/things to think about:
These are just some questions I had after reviewing the PR, but they are for the future. I know these were not in the scope of this PR 🙏 |
Yes, this is just initial work 👍
You you mean light/dark themes? Users can/should be able to do that by overriding a few "semantic tokens" (which do don't have yet) - they shouldn't need a "full palette" of their own
One more reason to now inject the "palette" tokens in |
fd69e38
to
4b70093
Compare
…mode refactor: update pentaho+ to use new palette chore: nested palette tokens chore: add palette colors to theme object
theme.palette
- values are not injected as CSS varspositive
,atmo<X>
) with the new palette in the thePentaho+
themebackgroundColor
/containerBackgroundHover
) to the tokens declaration