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

New Sass variable to handle <mark> dark mode bg color #39141

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Sep 3, 2023

Description

This PR allows to customize <mark> text and bg color in light and dark mode.

$mark-bg already existed but wasn't configurable in dark mode so $mark-bg-dark has been created with a default yellow-800 and associated to var(--bs-highlight-bg) in dark mode (like it's the case in light mode).

Depending on the value chosen as bg color, we have to introduce the customization of <mark> text color via 2 new Sass variables ($mark-color and $mark-color-dark) that are used to define a new custom property var(--bs-highlight-color) use to define mark color.

This is in the same spirit as what we can do for code color, kbd color, etc.

This is not considered a new feature since it's rather considered as something missing in the color modes (dark mode) introduced in v5.3.0.

Motivation & Context

Originally discussed in https://github.com/orgs/twbs/discussions/39139 and seems logical since we have already $code-color-dark which is in the same spirit. FYI, we have the same kind of variable in Boosted (fork of Bootstrap for our dark variants).

Type of changes

  • Enhancement (non-breaking change which adds functionality)

Checklist

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

Live previews

Related discussion

https://github.com/orgs/twbs/discussions/39139

@julien-deramond julien-deramond marked this pull request as ready for review September 3, 2023 07:49
@julien-deramond julien-deramond requested a review from a team as a code owner September 3, 2023 07:49
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@julien-deramond julien-deramond changed the title New Sass variable to handle <mark> dark mode color New Sass variable to handle <mark> dark mode bg color Sep 11, 2023
@mdo
Copy link
Member

mdo commented Sep 12, 2023

I think some folks will call this a new feature since it's a new var, but I think it's also a bug to be addressed. Let's set the color to a darker yellow for dark mode maybe? Yellow-800/900?

@julien-deramond
Copy link
Member Author

I think some folks will call this a new feature since it's a new var, but I think it's also a bug to be addressed.

I'd say that Bootstrap v5.3 is the integration of the color modes (dark mode). Bootstrap v5.3.2 (or v5.3.3) are patches to fix missing variables/properties/concepts from this new big feature which is the color modes. So, these new Sass/CSS variables to handle <mark> were rather something missing than a new feature coming with a potential v5.4 IMO.

Let's set the color to a darker yellow for dark mode maybe? Yellow-800/900?

Here's the rendering with yellow-900:
Screenshot 2023-09-12 at 22 57 31

Here's the rendering with yellow-800:
Screenshot 2023-09-12 at 23 01 33

I find yellow-800 standing out more compared to yellow-900. Both were tested with a contrast color analyzer and need a white text color.

Depending on the values tested, it was necessary to add something to handle the mark text color. This PR will probably need the following modification as well if we want to provide some freedom to our users:

diff --git a/scss/_reboot.scss b/scss/_reboot.scss
index 81ea21617..18791753d 100644
--- a/scss/_reboot.scss
+++ b/scss/_reboot.scss
@@ -217,6 +217,7 @@ small {
 
 mark {
   padding: $mark-padding;
+  color: var(--#{$prefix}highlight-color);
   background-color: var(--#{$prefix}highlight-bg);
 }
 
diff --git a/scss/_root.scss b/scss/_root.scss
index 35be03ae5..becddf14a 100644
--- a/scss/_root.scss
+++ b/scss/_root.scss
@@ -91,6 +91,7 @@
   }
 
   --#{$prefix}code-color: #{$code-color};
+  --#{$prefix}highlight-color: #{$mark-color};
   --#{$prefix}highlight-bg: #{$mark-bg};
 
   // scss-docs-start root-border-var
@@ -171,6 +172,7 @@
     --#{$prefix}link-hover-color-rgb: #{to-rgb($link-hover-color-dark)};
 
     --#{$prefix}code-color: #{$code-color-dark};
+    --#{$prefix}highlight-color: #{$mark-color-dark};
     --#{$prefix}highlight-bg: #{$mark-bg-dark};
 
     --#{$prefix}border-color: #{$border-color-dark};
diff --git a/scss/_variables-dark.scss b/scss/_variables-dark.scss
index 883858ec2..6422b3874 100644
--- a/scss/_variables-dark.scss
+++ b/scss/_variables-dark.scss
@@ -53,7 +53,8 @@ $headings-color-dark:               inherit !default;
 $link-color-dark:                   tint-color($primary, 40%) !default;
 $link-hover-color-dark:             shift-color($link-color-dark, -$link-shade-percentage) !default;
 $code-color-dark:                   tint-color($code-color, 40%) !default;
-$mark-bg-dark:                      $mark-bg !default;
+$mark-color-dark:                   $body-color-dark !default;
+$mark-bg-dark:                      $yellow-800 !default;
 
 
 //
diff --git a/scss/_variables.scss b/scss/_variables.scss
index 7706c0f6a..8b7437878 100644
--- a/scss/_variables.scss
+++ b/scss/_variables.scss
@@ -718,6 +718,7 @@ $dt-font-weight:              $font-weight-bold !default;
 $list-inline-padding:         .5rem !default;
 
 $mark-padding:                .1875em !default;
+$mark-color:                  $body-color !default;
 $mark-bg:                     $yellow-100 !default;
 // scss-docs-end type-variables

@mdo
Copy link
Member

mdo commented Sep 13, 2023

That all works for me!

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Still fine

@julien-deramond julien-deramond merged commit bde23ae into main Sep 13, 2023
@julien-deramond julien-deramond deleted the main-jd-handle-mark-in-dark-mode branch September 13, 2023 06:50
romankupchak93 pushed a commit to romankupchak93/bootstrap that referenced this pull request Jan 5, 2024
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