Skip to content

Fix duplicate --bs-emphasis-color set value#37809

Merged
mdo merged 2 commits intomainfrom
main-jd-drop-dupe-emphasis-color-css-var
Jan 6, 2023
Merged

Fix duplicate --bs-emphasis-color set value#37809
mdo merged 2 commits intomainfrom
main-jd-drop-dupe-emphasis-color-css-var

Conversation

@julien-deramond
Copy link
Copy Markdown
Member

@julien-deramond julien-deramond commented Jan 5, 2023

Description

--bs-emphasis-color is set 2 times in scss/_root.scss; 1 time with $body-emphasis-color and 1 time with $emphasis-color.

I supposed that the last is the right one based on what we fixed recently on dark mode (having a white color instead of $gray-100) so this PR suggests to have --bs-emphasis-color based on $emphasis-color on light mode and $emphasis-color-dark in dark mode.

It means that $body-emphasis-color and $body-emphasis-color-dark become useless and can be dropped.

@mdo I let you double-check if the rendering is not broken and if this is the way you thought the Sass/CSS vars. We can keep $body-emphasis-color* and remove $emphasis-color* if that makes more sense to you.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • (NA) My change introduces changes to the documentation
  • (NA) I have updated the documentation accordingly
  • (NA) I have added tests to cover my changes
  • All new and existing tests passed

Live previews

@julien-deramond julien-deramond mentioned this pull request Jan 5, 2023
81 tasks
@julien-deramond julien-deramond force-pushed the main-jd-drop-dupe-emphasis-color-css-var branch from 781cf30 to 3ee653e Compare January 5, 2023 08:29
@julien-deramond julien-deramond marked this pull request as ready for review January 5, 2023 08:30
@julien-deramond julien-deramond requested a review from a team as a code owner January 5, 2023 08:30
@mdo mdo force-pushed the main-jd-drop-dupe-emphasis-color-css-var branch from 3ee653e to 495d979 Compare January 6, 2023 03:34
Copy link
Copy Markdown
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Pushed a change to do the reverse. I included the body to first imply that this is an emphasis against the default body text, and second to match the naming of our new body-secondary and body-tertiary names.

Thanks for catching though!

@mdo mdo merged commit 702a3b6 into main Jan 6, 2023
@mdo mdo deleted the main-jd-drop-dupe-emphasis-color-css-var branch January 6, 2023 03:59
@doughlass
Copy link
Copy Markdown

@mdo I'm trying to rattle my brain to understand why the following was overwritten on scss/_root.scss.

@each $name, $value in $grid-breakpoints {
    --#{$prefix}breakpoint-#{$name}: #{$value};
  }

The code was merged back in October 2022 via #36095 and I've gone through commit history and found that this pull request changes the content what was already on _root.scss.

Please can we get this $grid-breakpoints back in as it was previously intended.

@mahilanmjd mahilanmjd mentioned this pull request Apr 16, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants