-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Portal Theme: Remove semantic color variables #2436
Portal Theme: Remove semantic color variables #2436
Conversation
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.
@yvonnesjy this works in all of the metacatui themes I tested and looks good! I have some questions:
elements in map-view.css
use color variables from the portals (e.g. color: var(--portal-col-neutral-00)
). However, maps exist outside of portals: in the main repository's data catalog view, and eventually on dataset landing pages. I'm not sure it makes sense to use portal colors in these contexts. Should we create a new set of color variables for the main repository and use those in map-view.css
? Do you have any suggestions about how best can best handle this as we try to improve metacatui's css and color theming in the future? (Same with portal color vars used in FeatureInfoView.js
)
^ Silly me, I wrote this before reading your doc, which explicitly states:
CSS rules in map-view.css should not refer to --portal-col. --map-col only.
and
Other pages should refer to portal variables, then main repo variables (future), then default.
I like this plan! ✅ ✅ ✅
So the only remaining question is:
When and how should we remove the __deprecate
css vars? Is this part of the plan for new color system?
Bahahha the doc is really just my scratchpad right now sorry you read through that! The notes are all from before we came up with the primary/neutral color concept, so it needs some updating.
This will be no longer true, and
This will be true for all pages, including map!
I think this will depend on the affected portals/main repos that rely on the default. It'd be best if we can first come up with a neutral enough primary/neutral palette. Then replace the __deprecate vars with --default-col-<primary/neutral>-x (if the affected maps are okay with the change). |
Ahh thanks for the explanation! Then I reinstate my initial question: Maps are not supposed to be portal specific, so how do we handle color theming in other contexts? I see the logic in "other pages should refer to portal variables, then main repo variables (future), then default", but as we add new contexts, do we keep adding to this list of fallback colors? Also, how do we re-use the styles from one context in another? e.g. how could we use the light map colors in the main data catalog repo? |
Besides the portal map page and main repo map page, what other context will we have in the future?
map-view.css currently doesn't consider main repo colors yet, but in the future, I imagine a set of --main-col-<primary/neutral>-x so map will use it as a fallback if the portal var doesn't exist (which will be the case on a main repo map). If we want to use the same light colors, we can just apply the same values to the --main-col-... set. |
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 you have a mapping from old -> new please share. My feature PR is going to run into come conflicts with this PR and I'll need to update the colors that I'm referencing
The changes should be all no-op. You can find the root values of the color variables in your PR, and just replace blue/grey with primary/neutral. Feel free to make the update first since it'll be easier to look up, and I can wait to merge. |
1. Replace current blue- and grey- color variables with primary- and neutral- 2. Remove all semantic color variables I created in the light theme update. Replace them with the primary- and neutral- variables that they reference. This PR is a part of the plan for new color system. design doc in progress: https://docs.google.com/document/d/1Eb0uixayVeEgO0AcIEjDO6J5555A_CA1BK0nZp1s-uk/edit?usp=sharing We're doing this refactoring first because it will make the legend work a lot simpler.
4259c8e
to
e13ac83
Compare
This PR is a part of the plan for new color system. design doc in progress: https://docs.google.com/document/d/1Eb0uixayVeEgO0AcIEjDO6J5555A_CA1BK0nZp1s-uk/edit?usp=sharing We're doing this refactoring first because it will make the legend work a lot simpler.