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

Dark theme fixes for navbar and dropdown (Fixes #39984) #39989

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

karlshea
Copy link

@karlshea karlshea commented May 15, 2024

Description

  • Allow data-bs-theme="dark" on parent element of .navbar to trigger .navbar-dark variables as described in the documentation.
  • Allow data-bs-theme="dark" on parent or current element of .dropdown-menu to trigger .dropdown-menu-dark variables as described in the documentation.

Motivation & Context

Closes #39984

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
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

#39984, #39291

@karlshea karlshea requested a review from a team as a code owner May 15, 2024 23:03
@karlshea karlshea changed the title Dark theme fixes Dark theme fixes for #39984 May 15, 2024
@karlshea karlshea changed the title Dark theme fixes for #39984 Dark theme fixes for navbar and dropdown (Fixes #39984) May 16, 2024
@julien-deramond julien-deramond self-requested a review May 25, 2024 06:18
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.

Thanks for submitting a PR @karlshea

I haven't looked at the code and the modifications that you suggest yet. However, I can see in the deployed version a rendering issue in the dark mode version of https://deploy-preview-39989--twbs-bootstrap.netlify.app/docs/5.3/components/navbar/#color-schemes, where the last navbar should render black texts instead of white:
Screenshot 2024-05-25 at 08 18 13

@karlshea
Copy link
Author

@julien-deramond I'm not sure how the last navbar could be fixed if the background color is set with a style tag, there doesn't seem to be anything to work off of there?

In 5.3 without this PR's change if I remove bg-primary from the second navbar and manually set the background color with a style the text also renders white in dark mode.

@julien-deramond
Copy link
Member

I've checked rapidly, but there is a forced data-bs-theme="light" (that should be displayed in the markup of the documentation, by the way). In this case, the color should be black since the light mode is forced.

I'll need to find the time to check all the existing PRs and issues at the same time regarding the color modes, because this could be also fixed automatically by #39295 if the color: is set by default (not the case right now).

@karlshea
Copy link
Author

Yeah I'm not really sure what to do here. Maybe CSS specificity is winning?

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.

Dark dropdowns/navbars don't get dark variables with dark theme
2 participants