-
Notifications
You must be signed in to change notification settings - Fork 0
feat: use blue not purple accent color (via core-styles) #1087
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
base: main
Are you sure you want to change the base?
feat: use blue not purple accent color (via core-styles) #1087
Conversation
Solution: - Delete old CSS vars. Why: - So Portal uses CSS vars from Core-Styles, instead of overwriting them. History: 1. Portal used Core-Styles v0 (only a few vars). 2. Portal defined its own CSS vars. 3. Wes copied the vars to Core-Styles v1. 4. Portal never used Core-Styles v1. 5. Portal upgraded to Core-Styles v2. 6. Portal retained those vars which became Core-Styles v1. 7. Portal is overwriting Core-Styles v2 vars with its v1 vars. 8. Thus, portal still uses purple, not blue.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1087 +/- ##
=======================================
Coverage 70.64% 70.64%
=======================================
Files 540 540
Lines 33727 33727
Branches 3002 3002
=======================================
Hits 23828 23828
Misses 9690 9690
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
wesleyboar
left a comment
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.
Notes for reviewers.
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.
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.
Use only Core-Styles settings/font(…).css (ref 1, ref 2); same values1 plus global font size vars.
Footnotes
-
Same values, eventually. You gotta follow the
--this-var: var(--that-var)cascade2 to verify. I verified. ↩ -
I will simplify the cascade in Core-Styles v3. ↩
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.
| @import url('@tacc/core-styles/dist/settings/border.css'); | ||
| @import url('@tacc/core-styles/dist/settings/color.css'); | ||
| @import url('@tacc/core-styles/dist/settings/color--portal.v2.css'); | ||
| @import url('@tacc/core-styles/dist/settings/font.css'); | ||
| @import url('@tacc/core-styles/dist/settings/font--portal.css'); | ||
| @import url('@tacc/core-styles/dist/settings/space.css'); |
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.
Basically, load the same CSS vars, with accent color change, plus some new font size vars that are innocuous and intended for Core-Portal to eventually use.
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.
See #1087 (review) for annotations.
taoteg
left a comment
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!
| --global-color-accent--weak: #6039cc40; /* similar to #E3D7FD on #f4f4f4 */ | ||
|
|
||
| --global-color-info--dark: var(--global-color-primary--x-dark); | ||
| --global-color-info--normal: var(--global-color-primary--normal); |
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.
fnets
left a comment
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, but a minor aesthetic change. If it's not important, I'll go ahead and approve this.
|
What @fnets found is minor, but —
— so please hold for now. |


Overview
Warning
Unexpected color change. A11y potentially compromised.
History
Related
Changes
Testing
UI