-
Notifications
You must be signed in to change notification settings - Fork 3
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
WRR-287: Replace CSS variables having R,G,B formats with standard #RGB formats #1754
base: develop
Are you sure you want to change the base?
Conversation
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…t-text-color Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…onent-text-sub-color Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…mponent-focus-text-color Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…olor Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…-color Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…-color Note that a new CSS variable --sand-keyguide-bg-color-alpha is also added to support the alpha channel of the background color. Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…overlay-bg-color Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…d opacity Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
…iables Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1754 +/- ##
===========================================
+ Coverage 80.81% 81.41% +0.60%
===========================================
Files 148 148
Lines 6681 6892 +211
Branches 1986 2081 +95
===========================================
+ Hits 5399 5611 +212
+ Misses 973 972 -1
Partials 309 309 ☔ View full report in Codecov by Sentry. |
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Alert/Alert.module.less
Outdated
--sand-progress-bg-color-alpha: @sand-alert-overlay-progress-bg-color-alpha; | ||
--sand-checkbox-color: @sand-alert-overlay-checkbox-color; | ||
--sand-checkbox-disabled-selected-text-color: @sand-alert-overlay-checkbox-disabled-selected-text-color; | ||
--sand-formcheckboxitem-focus-text-color: @sand-alert-overlay-formcheckboxitem-focus-text-color; | ||
--sand-item-disabled-focus-bg-color: @sand-alert-overlay-item-disabled-focus-bg-color; | ||
|
||
background-color: rgb(@sand-alert-overlay-bg-color-rgb, @sand-alert-overlay-bg-color-opacity); | ||
background-color: ~"color(from" @sand-alert-overlay-bg-color ~"srgb r g b /" @sand-alert-overlay-bg-color-alpha ~")"; |
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.
Is there a reason for renaming -opacity
to -alpha
?
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.
Discussing with @MikyungKim, we'd like to change color-alpha to color-opacity.
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.
Note: from now, -color-opacity for opacity of a color like text color or background color and -opacity (without color in front of it) for opacity of a component
@sand-alert-overlay-text-color-rgb: var(--sand-alert-overlay-text-color-rgb, 255, 255, 255); // #ffffff | ||
@sand-alert-overlay-bg-color-rgb: var(--sand-alert-overlay-bg-color-rgb, 0, 0, 0); // #000000 | ||
@sand-alert-overlay-bg-color-opacity: 75%; | ||
@sand-alert-overlay-bg-color: var(--sand-alert-overlay-bg-color, rgb(var(--sand-alert-overlay-bg-color-rgb, 0, 0, 0))); // #000000 |
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.
What is --sand-alert-overlay-bg-color-rgb
?
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.
It is a kind of fallback for old legacy uses. I will update the PR description.
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.
Updated
styles/colors.less
Outdated
@sand-component-text-sub-color-rgb: var(--sand-component-text-sub-color-rgb, 171, 174, 179); // #abaeb3 | ||
@sand-component-text-sub-color: rgb(@sand-component-text-sub-color-rgb); | ||
@sand-shadow-color: var(--sand-shadow-color, rgb(var(--sand-shadow-color-rgb, 0, 0, 0))); // #000000 | ||
@sand-shadow-common-color: ~"color(from" @sand-shadow-color ~"srgb r g b / 0.3)"; |
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.
Let's discuss the naming again.
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.
Conclusion: @sand-shadow-base-color for @sand-shadow-color, and @sand-shadow-color for @sand-shadow-common-color as the current @sand-shadow-common-color is the basic shadow color.
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.
done
styles/colors.less
Outdated
@@ -132,14 +123,15 @@ | |||
@sand-disabled-opacity: 0.28; // This value brings the standard text-color to the disabled text color specs | |||
@sand-disabled-bg-opacity: @sand-disabled-opacity; | |||
@sand-disabled-focus-content-opacity: 0.4; | |||
@sand-disabled-focus-content-color-alpha: 0.4; |
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.
Is this necessary?
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.
I will rename this also.
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.
done
styles/colors.less
Outdated
@sand-keyguide-bg-color: var(--sand-keyguide-bg-color, rgb(var(--sand-keyguide-bg-color-rgb, 55, 58, 65))); // #373a41 | ||
@sand-keyguide-bg-color-alpha: var(--sand-keyguide-bg-color-alpha, 0.9); |
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.
As we discussed let's rename @sand-keyguide-bg-color-alpha
to @sand-keyguide-bg-color
with color css function and @sand-keyguide-bg-color
to @sand-keyguide-bg-base-color
.
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.
I updated the code and it looks good to leave @sand-keyguide-bg-color-opacity since it makes easy to change opacity in high-contrast skin variants just like other components. Just like before this PR, @sand-keyguide-bg-color is the color with opacity value and Keyguide module directly uses this.
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
- @sand-shadow-color is renamed to @sand-shadow-base-color - @sand-shadow-common-color is renamed to @sand-shadow-color Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
- @sand-keyguide-bg-color is renamed to @sand-keyguide-base-bg-color - @sand-keyguide-bg-color is now the background color with opacity just like before this PR Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])
Checklist
Issue Resolved / Feature Added
CSS variables for customization of apps on run-time is introduced in 2.1.0 and we used some R,G,B format values for colors that need to be translucent on some UI components. At that time, we had no proper way to combine #RGB format with opacity value in CSS semantics.
As of now, we can use relative RGB semantics and it is a good time to turn non-standard R,G,B formats into standard #RGB formats.
Resolution
In this pull request, the followings are done.
Additional Considerations
Replaced CSS variables (corresponding LESS variables are also replaced)
--sand-text-color-rgb > --sand-text-color
--sand-shadow-color-rgb > --sand-shadow-color
--sand-component-text-color-rgb > --sand-component-text-color
--sand-component-text-sub-color-rgb > --sand-component-text-sub-color
--sand-focus-bg-color-rgb > --sand-focus-bg-color
--sand-component-focus-text-color-rgb > --sand-component-focus-text-color
--sand-selected-color-rgb > --sand-selected-color
--sand-overlay-bg-color-rgb > --sand-overlay-bg-color
--sand-progress-color-rgb > --sand-progress-color
--sand-progress-bg-color-rgb > --sand-progress-bg-color
--sand-keyguide-bg-color-rgb > --sand-keyguide-bg-color
--sand-alert-overlay-bg-color-rgb > --sand-alert-overlay-bg-color
--sand-alert-overlay-text-color-rgb > --sand-alert-overlay-text-color
--sand-alert-overlay-progress-color-rgb > --sand-alert-overlay-progress-color
--sand-alert-overlay-progress-bg-color-rgb > --sand-alert-overlay-progress-bg-color
Modified CSS variables
--sand-progress-bg-color-opacity is now a number between 0 and 1 instead of a percentage.
--sand-spinner-color is now #hex format instead of {R,G,B} format. (It was a bug that we did not name it --sand-spinner-color-rgb before).
--sand-alert-overlay-progress-bg-color-opacity is now a number between 0 and 1 instead of a percentage.
Renamed CSS variables (corresponding LESS variables are also renamed)
--sand-progress-bg-color-alpha > --sand-progress-bg-color-opacity
--sand-alert-overlay-progress-bg-color-alpha > --sand-alert-overlay-progress-bg-color-opacity
New CSS variables
--sand-keyguide-bg-color-opacity is added to support overriding.
New LESS variables
@sand-shadow-base-color is added as a base color without opacity.
@sand-keyguide-bg-base-color and @sand-keyguide-bg-color-opacity are added to override easily for skins.
@sand-disabled-focus-content-color-opacity is added to separate from @sand-disabled-focus-content-opacity that is used as an opacity for whole control not only text.
Legacy CSS variable support
Since this PR renames all -rgb named CSS variables, legacy code to add -rgb named CSS variables will not work without fallback. So, I added fallback like below.
This is the same as the follow if we don't need fallbacks.
Links
WRR-287
Comments
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek ([email protected])