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

feat: hide unimplemented dark mode for v5.3.0 #2044

Merged
merged 11 commits into from
Jul 21, 2023

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented May 22, 2023

Description

This PR will be the latest PR to merge just before the release of Boosted v5.3.0. It hides the dark mode as it is not yet implemented for Boosted. The inner mechanism will stay, but it must be deactivated from the documentation.

The content of this PR must be reverted right after the release so we can get back to work based on the main branch of Bootstrap. If a v5.3.1 is released before we have the time to release the dark mode, we'll get back this PR again.

Detailed content of this PR in terms of modifications:

Motivation & Context

The readers/users must understand by reading the documentation (migration guide included) that the inner mechanism for the color mode is available but they can't use it yet for the dark mode in their projects.

Types of change

  • New feature (non-breaking change which adds functionality)

Live previews

@netlify
Copy link

netlify bot commented May 22, 2023

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit f01a65e
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/64b933461718060008453f1a
😎 Deploy Preview https://deploy-preview-2044--boosted.netlify.app/docs/5.3/migration
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sonarcloud
Copy link

sonarcloud bot commented May 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

Should we remove all the references to data-bs-theme="*" and color mode in the doc ? (https://deploy-preview-2044--boosted.netlify.app/docs/5.3/components/navbar/#external-content, https://deploy-preview-2044--boosted.netlify.app/docs/5.3/components/navbar/#color-schemes, https://deploy-preview-2044--boosted.netlify.app/docs/5.3/migration/). Imo it could be a bit weird to reference something that people can't use.

Should we really keep the import of variables-dark.scss? Imo it could be hidden in the doc since we disable it via $enable-dark-mode: false ass you mention. I don't know the way people are using Boosted but it seems like Bootstrap has some trouble with it, so maybe we can retain it for now and see ?

In the scss, I think we are fine since it doesn't impact the bundle if the $enable-dark-mode is set to false. The main things are in the docs CSS.

For the color-mode experience, I agree that we might need a callout on the color modes page but maybe we can add an informative callout that it'll come until v5.4 or asap or something ? Otherwise, we could maybe retouch some parts of the page like the homemade color mode and the javascript part ?

I couldn't think about something you didn't mentionned in the description so well done :)

@julien-deramond julien-deramond marked this pull request as ready for review July 20, 2023 11:48
@julien-deramond
Copy link
Member Author

julien-deramond commented Jul 20, 2023

Finally chose to keep $enable-dark-mode set to true because it breaks examples in the "Colors modes" page and wouldn't be compatible with plugins and libraries based on Bootstrap v5.3.0. Might be more difficult to understand for the users, and some might be angry, but compatibility would be ensured. On top of that, it will be easier for us to maintain rather than having to revert a ton of modifications in the documentation.

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 🚀 I don't think we can handle much apart warning users, let's give it a try !

@julien-deramond julien-deramond merged commit 235a887 into main Jul 21, 2023
12 checks passed
@julien-deramond julien-deramond deleted the main-jd-final-pr-before-v5.3.0-stable branch July 21, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants