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

Simplify CSS variable naming / structure #623

Closed
12rambau opened this issue Apr 14, 2022 · 2 comments · Fixed by #659
Closed

Simplify CSS variable naming / structure #623

12rambau opened this issue Apr 14, 2022 · 2 comments · Fixed by #659

Comments

@12rambau
Copy link
Collaborator

In a recent PR (#540 (comment)) @choldgraf suggested that the number of custom variable should be reduced:

One thing I'm feeling as I just went through the codebase (and echoing @pradyunsg's thoughts): we have way too many custom-defined color variables. In a subsequent PR, I think we should just define a handful of colors (e.g. "base", "primary", "secondary", etc, and just re-use those throughout)

would you thing it would make sense to go back to what material design is suggesting ?

Note: to maintain a correct dark theme some specific variables will still be needed (on_background, on_surface, shadow) in addition to the classic one (primary, secondary etc)

@choldgraf choldgraf changed the title open discussion about css variables Simplify CSS variable naming / structure Apr 15, 2022
@choldgraf
Copy link
Collaborator

I don't have strong opinions about CSS variables in general, but that partly leads to my stronger opinion:

I think that we should adopt the minimal number of CSS variables possible, and make them follow some pre-existing standard (I think the material theme approach sounds reasonable). Otherwise I worry that it will grow in complexity and be hard to maintain.

@choldgraf
Copy link
Collaborator

@12rambau a million thanks for your help on this

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 a pull request may close this issue.

2 participants