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

Remove deprecated sass color-#{state} variables usage #3853

Merged

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Dec 3, 2020

Description

This is a preparatory PR for #3816 and needs backport.

With 9552ccc we deprecated the following variables in favor of their boostrap counterpart:

Deprecated New
$color-success theme-color(success)
$color-notice theme-color(warning)
$color-error theme-color(danger)

This PR is quite complex because we were using these deprecated variables in variables.scss itself and we can't just swap them to the new version since when this file is imported we didn't import the Boostrap file yet. And there's a reason for that: we define Boostrap colors using the variables defined in variables.scss. 🤯

I propose to move the actions related variable in the already existing actions.scss file, so that we can define them later, after Boostrap files have been loaded.

We preserved the ability to customize those variables per store because they will be still loaded after the variables_override.scss file.

There's some slight visual change in the colors, due to using the boostrap built-in theme-color-level() function, instead of sass lighten():

Flash Messages

Screenshot 2020-12-03 at 18 15 06 |

Table rows with highlighted colors

Success Warning Danger
Screenshot 2020-12-03 at 16 47 33 Screenshot 2020-12-03 at 16 47 49 Screenshot 2020-12-03 at 16 47 59
Checklist:
  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have attached screenshots to this PR for visual changes (if needed)

At the moment they are using deprecated varaibles while we
should use the bootstrap variables instead.

Having them into the main variables.css file makes this
impossible since bootstrap variables are imported after that file.

We can't import the bootstrap file before the variables one because
bootstrap varaibles are actually using our custom variables.
You can read the value without the need of a multiplication now.
This is not a 1-1 mapping with what we had before but it's very similar.
Instead of $color-success, which is deprecated. They contain
the same value and this change has no effect on how the css
will look like.

$green is what bootstrap uses internally to populate success
theme color, that is what we want to use instead of $color-green.
brand-something doesn't exist anymore in boostrap. It's now
accessible with theme-color(something).
@kennyadsl kennyadsl added this to the 2.11 milestone Dec 3, 2020
@kennyadsl kennyadsl self-assigned this Dec 3, 2020
@kennyadsl kennyadsl requested a review from a team December 7, 2020 17:15
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Very great work!

@kennyadsl kennyadsl merged commit 3382105 into solidusio:master Dec 10, 2020
@kennyadsl kennyadsl deleted the kennyadsl/remove-deprecated-variables-usage branch December 10, 2020 10:22
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.

3 participants