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

Color mode: Customization of dark theme #38136

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Feb 28, 2023

Description

Add a new maps and associated Sass variables to allow a deeper customization.
Initialized it to what we have atm.

Motivation & Context

Facilitate a deeper customization of the dark mode basic colors.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

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

Live previews

Related issues

Closes #39484.

@julien-deramond
Copy link
Member

julien-deramond commented Mar 17, 2023

I haven't had the time to review it yet but I agree that it's needed. For example, see https://github.com/orgs/twbs/discussions/38255 and https://github.com/orgs/twbs/discussions/38259. Or simply in our Orange fork where our primary color changes depending on light and dark mode for a11y (contrasts) reasons.

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

First batch of comments. I haven't checked everything yet.
As said in my previous comment, I think this is mandatory for our color modes so that projects can switch the color palette in dark mode for a11y reasons.

Targeted version

IMO, this PR should be embedded directly in v5.3.2 instead of v5.4.0 in order to finalize the color modes in v5.3.x.

Bundle size

main branch:

PASS ./dist/css/bootstrap-reboot.css: 3.26KB < 3.5KB (gzip)
PASS ./dist/css/bootstrap-reboot.min.css: 3.1KB < 3.25KB (gzip)
PASS ./dist/css/bootstrap-utilities.css: 11.5KB < 11.75KB (gzip)

main-lmp-dark-theme-customization branch:

PASS ./dist/css/bootstrap-reboot.css: 3.27KB < 3.5KB (gzip)
PASS ./dist/css/bootstrap-reboot.min.css: 3.11KB < 3.25KB (gzip)
PASS ./dist/css/bootstrap-utilities.css: 11.51KB < 11.75KB (gzip)

This slight size change is acceptable IMO and is coming from the generation of the following custom properties in dark mode (already existing in light mode):

>   --bs-primary: #0d6efd;
>   --bs-secondary: #6c757d;
>   --bs-success: #198754;
>   --bs-info: #0dcaf0;
>   --bs-warning: #ffc107;
>   --bs-danger: #dc3545;
>   --bs-light: #f8f9fa;
>   --bs-dark: #212529;
>   --bs-primary-rgb: 13, 110, 253;
>   --bs-secondary-rgb: 108, 117, 125;
>   --bs-success-rgb: 25, 135, 84;
>   --bs-info-rgb: 13, 202, 240;
>   --bs-warning-rgb: 255, 193, 7;
>   --bs-danger-rgb: 220, 53, 69;
>   --bs-light-rgb: 248, 249, 250;
>   --bs-dark-rgb: 33, 37, 41;

Documentation

Here’s probably something to update:

Issues

Have you checked whether this PR can close some existing issues in the tracker?

scss/_variables-dark.scss Outdated Show resolved Hide resolved
@julien-deramond
Copy link
Member

Another thing that is pretty problematic and that will maybe force us to land this PR in v6 only is that when $theme-colors-dark primary color is modified to be green for example in dark mode, buttons are not modified because they are defined with a loop over $theme-colors. This loop should use custom properties instead, but doing that will break the compilation because of what we're doing in button-variant() mixin with the use of tint-color, shade-color.

As discussed, @louismaximepiton, I let you create a branch based on this one to check what would be the necessary changes in order to make the buttons dependent on $theme-colors-dark as well. From that, we'll decide if this PR could be merged or not in v5.

@louismaximepiton louismaximepiton marked this pull request as draft October 23, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Primary, Secondary, and Success Outline Buttons are hard to view in Dark mode
2 participants