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

Enable sass parser in ui/build to parse indirect color assignments #16611

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

FawzyAshraf
Copy link
Contributor

The sass build for colors currently only works for direct assignments, but doesn't work correctly when being assigned to another variables and causes the assignee to be #000 instead.

Should fix #16559.

The sass build for colors currently only works for direct assignments,
but doesn't work correctly when being assigned to another variables and
causes the assignee to be #000 instead.
@ornicar ornicar requested a review from schlawg December 18, 2024 09:21
@schlawg
Copy link
Collaborator

schlawg commented Dec 18, 2024

@FawzyAshraf ui/build color mixing is a temporary (though long duration) hack we will gleefully abandon once an overwhelming majority of iphones and ipads are at or above Safari 16.4. At that point, we migrate to CSS Color Module Level 4 functions (i.e. color_mix) provided by the browser.

While this is an ingenious fix and I'm fine with merging it - the initial problem is that the definition of $c-good violates the requirements outlined in the preceding comment. that mixable colors such as $c-good must be defined as valid css (no scss variables allowed).

This change to ui/build heads us down the path of loosening that restriction and further into the forest of replicating scss features. But it seems safe, clean, and effective so I'm fine with it.

Regardless of this change:

Let's correct $c-good by duplicating the value for $c-secondary in code so as not to demonstrate a blatant violation of the comment restriction (which if honored, will protect us from bad things like this). for example:

$c-secondary: hsl(88, 62%, 37%);
$c-good: hsl(88, 62%, 37%); // yes it's duped, this is fine

Copy link
Collaborator

@schlawg schlawg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requesting updated definition for $c-good in /ui/common/css/theme/_default.scss then we're good to go.

@FawzyAshraf
Copy link
Contributor Author

@schlawg Thanks for the detailed comment, it was really helpful.

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 this pull request may close these issues.

Incorrect good move color
2 participants