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

Don't redefine $list-group-color in loop #33870

Merged
merged 1 commit into from May 10, 2021
Merged

Don't redefine $list-group-color in loop #33870

merged 1 commit into from May 10, 2021

Conversation

mhw
Copy link
Contributor

@mhw mhw commented May 7, 2021

The loop in scss/_list_group.scss uses $list-group-color as though it was a local variable, but is in fact overwriting the setting from _variables.scss. This means that using the value of $list-group-color in other stylesheets included in the scss build will get the wrong color value (I'd expect it to be #212529, but it is in fact #141619).

@mhw mhw requested a review from a team as a code owner May 7, 2021 09:03
@ffoodd
Copy link
Member

ffoodd commented May 7, 2021

Thanks! Confirmed in a Sassmeister.

If we're into this, $list-group-background doesn't match a global variable (the global one is $list-group-bg) so its override is not needed. But we could also harmonize naming in the loop to use $list-group-variant-bg. Does that sound accurate?

@mhw
Copy link
Contributor Author

mhw commented May 7, 2021

Yep, that's right: I renamed $list-group-background in the loop for consistency between the two "local" variables, not that it clashes with anything else. I'll update to $list-group-variant-bg as I can see that's more inline with the conventions.

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

@ffoodd
Copy link
Member

ffoodd commented May 7, 2021

As a sidenote we should check if we're not doping the same error somewhere else.

@mhw
Copy link
Contributor Author

mhw commented May 7, 2021

Yes, that's a good idea. I did a quick visual check with egrep '^ +\$' scss/* and didn't spot any other obvious occurrences, but I don't know the code base well enough to consider that exhaustive.

@ffoodd
Copy link
Member

ffoodd commented May 7, 2021

Thanks for trying though :)

@XhmikosR XhmikosR requested a review from mdo May 10, 2021 18:15
@XhmikosR XhmikosR added this to Inbox in v5.0.1 via automation May 10, 2021
v5.0.1 automation moved this from Inbox to Approved May 10, 2021
@mdo mdo merged commit b074e23 into twbs:main May 10, 2021
v5.0.1 automation moved this from Approved to Done May 10, 2021
@XhmikosR XhmikosR changed the title Don't redefine $list-group-color in loop Don't redefine $list-group-color in loop May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.1
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants