Skip to content

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
Contributor

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
Contributor

@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.

@ornicar ornicar merged commit 4082e63 into lichess-org:master Dec 19, 2024
3 checks passed
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

3 participants