-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Allow fewer breakpoints for navbars #38909
base: main
Are you sure you want to change the base?
Conversation
df80866
to
f703036
Compare
Signed-off-by: pine3ree <pine3ree@gmail.com>
Signed-off-by: pine3ree <pine3ree@gmail.com>
f703036
to
55dffaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this can help people, I see no reason why it shouldn't be added. It doesn't increase the bundle and offer more possibility to customize the framework.
Thanks for this PR!
|
||
// scss-docs-start navbar-breakpoints | ||
$navbar-breakpoints: $grid-breakpoints !default; | ||
// scss-docs-end navbar-breakpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never used, should be removed or used imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @louismaximepiton. The extra sass var is used to keep the two breakpoints separate. If we alter the $grid-breakpoints
map before the inclusion of _maps
we implicitly alter the $navbar-breakpoints
as well. If we remove only from $navbar-breakpoints
we can keep the $grid-breakpoints
variable as defined inside the configuration file. e.g.
- Define before the default value
// @import...
$navbar-breakpoints: map-remove($grid-breakpoints, "xl", "xxl");
@import "../node_modules/bootstrap/scss/maps";
// @import...
- Override (after) the default value
// @import...
@import "../node_modules/bootstrap/scss/maps";
$navbar-breakpoints: map-remove($navbar-breakpoints, "xl", "xxl");
// @import...
Now that I think about it, maybe it would be better to define the $navbar-breakpoints: $grid-breakpoints !default;
in the main variables configuration file, like I did in the first version. Otherwise I would need to change the added documentation, that could lead to confusion.
kind regards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I'm a bit confused,
I think I didn't explain well my thought on this.
I mean the change you mention is totally fine. My comment was more about the // scss-docs*
thing which isn't used in the doc. Or did I miss something ?
Sorry for the confusion and thanks again for the appreciated contribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@louismaximepiton Ah, ok. :-) I thought you meant the actual variable...not the docs. But I think it would make more sense either:
- to move the extra variable into the main configuration file, or
- to simply keep only the instruction to define the navbar-breakpoints using the grid-breakpoints var (as in my previous example 1) according to bootstraps docs that recommend to add/remove to/from maps before including the
_maps.scss
file.
kind regards
maps file Signed-off-by: pine3ree <pine3ree@gmail.com>
Description
Add the ability to remove navbar breakpoints
(Rebased PR replacing #36521)
Motivation & Context
We usually do not need so many breakpoints for navbars as for grids: see #36517 for feature description.
Type of changes
Checklist
npm run lint
)Related issues
Related issue/discussion