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

Missing $primary-text-emphasis-dark variable #38683

Closed
3 tasks done
jmtt89 opened this issue May 31, 2023 · 21 comments
Closed
3 tasks done

Missing $primary-text-emphasis-dark variable #38683

jmtt89 opened this issue May 31, 2023 · 21 comments
Labels

Comments

@jmtt89
Copy link

jmtt89 commented May 31, 2023

Prerequisites

Describe the issue

The problem is your update a minor version (5.2.3 -> 5.3.0) and have a very little Breaking Change

variable $primary-text-emphasis-dark not exist on 5.2.3 and is required on 5.3.0

SassError: Undefined variable.
   ╷
55 │     "primary": $primary-text-emphasis-dark,
   │                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  node_modules/bootstrap/scss/_maps.scss 55:16  @import
  src/styles.scss 9:9                           root stylesheet

The fix is very easy to implement, just need include the new file variables-dark on main styles

@import "../node_modules/bootstrap/scss/variables";
@import "../node_modules/bootstrap/scss/variables-dark";
 ...
@import "../node_modules/bootstrap/scss/maps";

Some automated process may throw an error, because it's a minor update and it should be backwards compatible.

Reduced test cases


What operating system(s) are you seeing the problem on?

Windows

What browser(s) are you seeing the problem on?

Microsoft Edge

What version of Bootstrap are you using?

5.3.0

@julien-deramond
Copy link
Member

julien-deramond commented May 31, 2023

Haven't tested it and IDK if it's something we can actually do; but maybe we could consider moving the content of _variables-dark.scss at the end of _variables.scss (since the Sass var names are all different) surrounded by a @if $enable-dark-mode in a v5.3.1 so that there's no need to import a new file while migrating. And reintroduce this _variables-dark.scss only for v6.

One other option would be to import _variables-dark.scss at the end of our _variables.scss. Not very consistent with the way we handle Sass files currently we could allow us to keep separate files and limit the breaking change.
(Haven't tried it)

@twbs/css-review thoughts?

@mdo
Copy link
Member

mdo commented May 31, 2023

Minor updates have always allowed for new features, and most new features come with new files (e.g., adding toasts). I think having these new ones as separate stylesheets is fine. Maybe there's more we can do to document the migration?

@palmtown
Copy link

palmtown commented Jun 1, 2023

+1

I ran into the same issue. I included the _variables-dark.scss, however, ran into another issue thereafter as shown at #38687.

@Skowronek-Jaskula
Copy link

This is a breaking change actually. I have to change imports in my project.

@mdo
Copy link
Member

mdo commented Jun 6, 2023

Closing as this isn't a breaking change—minor releases can include new features. This has always been the case in Bootstrap.

@mdo mdo closed this as not planned Won't fix, can't repro, duplicate, stale Jun 6, 2023
@jmtt89
Copy link
Author

jmtt89 commented Jun 6, 2023

ok, if the bootstrap team disagrees and decides to force the people to update their code, that's fine, it's a very easy fix, but don't deny the facts...

In some circumstances, the 5.3.0 update breaks projects running 5.2.3... so yes, it's a Breaking Change because the update 5.3.0 is not safe to apply... a minor change requirement is broken... The Backwards Compatibility.

References:

KBeDevel added a commit to stratego-chile/site that referenced this issue Jun 6, 2023
cloudkucooland added a commit to cloudkucooland/wado-firebase that referenced this issue Jun 14, 2023
@julien-deramond julien-deramond removed this from v5.3.1 Jun 15, 2023
@AdelinGhanaem
Copy link

As a backend developer trying to ease my life with these libraries but I end up putting on more work with these versions, modules, and all this crap!

@unleashit
Copy link

How isn't this a breaking change? I came here because my build was broken after upgrading. I ended up pinning it to 5.2.3 since importing the dark vars caused other errors and I don't have time to deal with it yet. Had to pin, since npm install on ^5.2.3 still upgrades to 5.3.0.

Side note: I can't wait for the dark mode obsession trend to pass Hopefully with something a little more nuanced, say a 50 shades of grey mode 😆

@ethanclevenger91
Copy link
Contributor

Minor updates have always allowed for new features, and most new features come with new files (e.g., adding toasts). I think having these new ones as separate stylesheets is fine. Maybe there's more we can do to document the migration?

The difference here is that the end user can opt into toasts, and not having toasts enabled does not break existing builds. This update broke existing builds, and therefore is a breaking change. It's not about adding the feature, it's about the implementation.

To reiterate what was already said - the fix was very straightforward. But shouldn't have been necessary at all.

@glitchwizard
Copy link

yeah this is a pretty breaking change if you ask me.... gonna throw down on that.

@confuzeus
Copy link

As a backend developer trying to ease my life with these libraries but I end up putting on more work with these versions, modules, and all this crap!

Lol yes, I have to come to github issue tracker every time I deploy my app now.

@ajaypatelaj
Copy link
Contributor

Yes, this should be considered for the next release as it breaks our project if we upgrade to v5.3.0

stdavis added a commit to agrc/wfrc-rtp-projects that referenced this issue Jul 3, 2023
stdavis added a commit to agrc/wfrc-rtp-projects that referenced this issue Jul 4, 2023
@TobiDimmel
Copy link

For people who don't want to use dark mode: you can also set $enable-dark-mode: false;.
Then you don't have to import bootstrap/scss/variables-dark.

@rgoupil
Copy link

rgoupil commented Aug 28, 2023

Closing as this isn't a breaking change—minor releases can include new features. This has always been the case in Bootstrap.

Adding a new, entirely optional, feature is completely fine.
This here, however, is not the case as additional steps are required to allow builds of existing system to pass, which is the definition of a breaking change.
I doubt linux would still be a thing today with this kind of policy on breaking changes, I am saddened to see Bootstrap no longer follow semver.

guhur added a commit to MieuxVoter/majority-judgment-web-app that referenced this issue Aug 29, 2023
@tomazartack
Copy link

As a backend developer trying to ease my life with these libraries but I end up putting on more work with these versions, modules, and all this crap!

I share your feelings there mate 👯‍♂️

@fercomunello
Copy link

@tomazartack Me too xD

@neonexus
Copy link

neonexus commented Nov 3, 2023

This was a real thorn in my side. There was no easy / clear way to fix the problem, and after a simple update. Why this was closed as not a "breaking change" blows my mind. It VERY MUCH is a breaking change, because a simple update broke my entire build.

I understand adding functionality, that's great. But if I'm forced to add in a new file, even if I don't intend to use it, I would consider that a breaking change.

@Michael-Ziluck
Copy link

@mdo I'm going to reopen a new ticket with the same description as there seems to be a misunderstanding on what "breaking change" means. It should mean: "if you update this library to a new version, all usages of said library should continue to compile."

Before version 5.3.0, the following code compiles:

@import "bootstrap/scss/functions";
@import "bootstrap/scss/variables";
@import "bootstrap/scss/mixins/breakpoints";

After version 5.3.0, the aforementioned code throws the following error:

HookWebpackError: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
Undefined variable.
   ╷
55 │     "primary": $primary-text-emphasis-dark,
   │                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  bootstrap\scss\_maps.scss 55:16  @import
  src\styles.scss 19:9             root stylesheet

If you disagree, can you please explain how you and the rest of the Bootstrap team interprets the phrase "breaking change"?

@SomnaW
Copy link

SomnaW commented Dec 12, 2023

The documentation at https://getbootstrap.com/docs/5.2/customize/sass, which is source controlled at bootstrap/site/content/docs/5.2/customize
/sass.md
needs to be updated to reflect this change. I'm chasing my tail because it isn't working when the official documentation doesn't mention the dark variables file at all. I'm glad I found the answer, but the docs should be updated so other people don't continue to make the same mistake.

@julien-deramond
Copy link
Member

@SomnaW variables-dark.scss was only introduced in v5.3.0 so it's normal not to find it in the v5.2 documentation.

datendelphin pushed a commit to fossgis/tileserver-web that referenced this issue Dec 31, 2023
@Sahilbal
Copy link

Sahilbal commented Feb 6, 2024

+1

I ran into the same issue. I included the _variables-dark.scss, however, ran into another issue thereafter as shown at #38687.

where and how should I add _variables-dark.scss file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests