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

Fix Sass compilation breaking change in v5.3 #39380

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Nov 8, 2023

Description

This PR hides a Sass compilation-breaking change happening with the new color modes feature. This fix should be removed in v6 as the breaking change will be tolerated.

This solution has two objectives:

  • users migrating from v5.2 to v5.3 don't have to add an extra import to their Sass code
  • users having followed the migration note in v5.3 don't have to remove the extra import in v5.3.3

So the idea here is to keep the documentation as is where it is mentioned that the extra import is needed.
However, our _variables.scss will automatically add the extra import if the user has not done it.

It shouldn't be an issue since variables would be just overwritten with the same content, and so the final bundle would remain the same.

To avoid other breaking changes linked to this same issue, a new scss/tests/mixins/_auto-import-of-variables-dark.test.scss has been created. I've voluntarily not modified other test files to make this new test independant so it can be removed safely in v6.

When reviewing this PR, please try to re-do the manual tests, and don't hesitate to comment on this PR with other use cases I've probably missed, just to be sure to cover everything.

Manuel testing

I've manually tested some use cases based on the following HTML file:

<!doctype html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Bootstrap w/ Vite</title>
  </head>
  <body>
    <div data-bs-theme="light">
      <button type="button" class="btn btn-outline-primary">Primary</button>
    </div>
    <div data-bs-theme="dark" class="bg-dark">
      <button type="button" class="btn btn-outline-primary">Primary</button>
    </div>
    <div data-bs-theme="light">
      <span class="text-primary fw-bold">TEST</span>
      <span class="text-primary-emphasis fw-bold">TEST</span>
    </div>
    <div data-bs-theme="dark" class="bg-dark">
      <span class="text-primary fw-bold">TEST</span>
      <span class="text-primary-emphasis fw-bold">TEST</span>
    </div>
    <script type="module" src="./js/main.js"></script>
  </body>
</html>

Simple Bootstrap import

@import "bootstrap";
  • Sass compilation OK
  • Buttons and text are blue

Simple Bootstrap import + primary color override

$primary: red;
@import "bootstrap";
  • Sass compilation OK
  • Buttons and text are red
Customized Bootstrap import
@import "../node_modules/bootstrap/scss/functions";
@import "../node_modules/bootstrap/scss/variables";
@import "../node_modules/bootstrap/scss/maps";
@import "../node_modules/bootstrap/scss/mixins";
@import "../node_modules/bootstrap/scss/root";
@import "../node_modules/bootstrap/scss/utilities";
@import "../node_modules/bootstrap/scss/reboot";
@import "../node_modules/bootstrap/scss/utilities/api";
@import "../node_modules/bootstrap/scss/buttons";
  • Sass compilation OK
  • Buttons and text are blue
Customized Bootstrap import + colors override
@import "../node_modules/bootstrap/scss/functions";

$primary: red;
$primary-text-emphasis-dark: yellow;
$primary-text-emphasis-dark: pink;

@import "../node_modules/bootstrap/scss/variables";
@import "../node_modules/bootstrap/scss/maps";
@import "../node_modules/bootstrap/scss/mixins";
@import "../node_modules/bootstrap/scss/root";
@import "../node_modules/bootstrap/scss/utilities";
@import "../node_modules/bootstrap/scss/reboot";
@import "../node_modules/bootstrap/scss/utilities/api";
@import "../node_modules/bootstrap/scss/buttons";
  • Sass compilation OK
  • Buttons and basic text are red
  • Other texts are dark yellow and pink
Customized Bootstrap import + colors override when _variables-dark.scss is imported
@import "../node_modules/bootstrap/scss/functions";

$primary: red;
$primary-text-emphasis-dark: yellow;
$primary-text-emphasis-dark: pink;

@import "../node_modules/bootstrap/scss/variables";
@import "../node_modules/bootstrap/scss/variables-dark";
@import "../node_modules/bootstrap/scss/maps";
@import "../node_modules/bootstrap/scss/mixins";
@import "../node_modules/bootstrap/scss/root";
@import "../node_modules/bootstrap/scss/utilities";
@import "../node_modules/bootstrap/scss/reboot";
@import "../node_modules/bootstrap/scss/utilities/api";
@import "../node_modules/bootstrap/scss/buttons";
  • Sass compilation OK
  • Buttons and basic text are red
  • Other texts are dark yellow and pink

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)
  • (N/A) My change introduces changes to the documentation
  • (N/A) I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Closes #39367

@julien-deramond julien-deramond force-pushed the main-jd-fix-sass-compilation-breaking-change branch from f25b1fd to 570cd24 Compare November 8, 2023 20:49
@julien-deramond julien-deramond marked this pull request as ready for review November 8, 2023 21:05
@julien-deramond julien-deramond requested a review from a team as a code owner November 8, 2023 21:05
@neonexus
Copy link

neonexus commented Nov 9, 2023

I have confirmed that this does in-fact correct the build-breaking issue, and does not have any affect on @import "~bootstrap/scss/variables-dark"; usage within a third-party build. Size remains the same either way, since they are just variables.

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.

I think it's small enough to get through as is since this behavior was introduced in v5.3 and we don't recommend overriding variables by using !default flag (couldn't find any workaround that doesn't break anything that was working before). However, one thing that doesn't work anymore is defining Sass variables for dark mode with !default flag between variables and variables-dark:

@import "variables";
$primary-text-emphasis-dark: #af6 !default; // Or a file containing this kind of variables
@import "variables-dark";

Despite of this small use case, I'd be fine to go with it.

A quick question, what the tests (@include describe(...)) stand for in this context ? If I remove the tests and keep the imports, I get the exact same results.

@julien-deramond
Copy link
Member Author

However, one thing that doesn't work anymore is defining Sass variables for dark mode with !default flag between variables and variables-dark [...] Despite of this small use case, I'd be fine to go with it.

Good catch, I didn't test this use case. However, I agree with you that it's really an edge case that shouldn't happen since it's not really logical to do that.

A quick question, what the tests (@include describe(...)) stand for in this context ? If I remove the tests and keep the imports, I get the exact same results.

You're right, I've simplified the test to rely only on imports and renamed it to _auto-import-of-variables-dark.test.scss.
The description is also modified to reflect this change.

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.

I'm fine with this version 👌

@julien-deramond julien-deramond merged commit 587e89f into main Nov 14, 2023
14 checks passed
@julien-deramond julien-deramond deleted the main-jd-fix-sass-compilation-breaking-change branch November 14, 2023 07:01
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
None yet
Development

Successfully merging this pull request may close these issues.

Reopened: Missing $primary-text-emphasis-dark variable
4 participants