Update nav link & pill items height#42146
Conversation
Currently, `link` and `nav-pills` items height sit at `36px` (because they don't have any borders), while `tab`, and `underline` sit at `38px`, which perfectly matches the height of `input` and `button` components. This PR adds an invisible border for the `link` and `nav-pills` items, so that all 4 `nav` variants share the exact same height, regardless of which is used, which makes it suitable to be used with other components, such as `input` or `buttons` for example, as described in this discussion: https://github.com/orgs/twbs/discussions/42145 without having discrepancy when it comes to elements height.
|
As a side note, the |
|
Second side note... I've noticed that: in the Not sure what's your take on this but I feel like defaulting them to the root I can also do a PR / patch with this change as well: Just wanted to ask first if that's something you'd consider. |
mdo
left a comment
There was a problem hiding this comment.
Thanks for the PRs, these have been super helpful!
scss/_nav.scss
Outdated
| border-inline: 0; | ||
| border-block-start: 0; | ||
| border-block-end: var(--nav-underline-border-width) solid transparent; |
There was a problem hiding this comment.
Shorten this down a bit maybe?
| border-inline: 0; | |
| border-block-start: 0; | |
| border-block-end: var(--nav-underline-border-width) solid transparent; | |
| border-width: 0; | |
| border-block-end: var(--nav-underline-border-width) solid transparent; |
There was a problem hiding this comment.
This doesn't work, as border-width is resetting the physical border.
So when we set border-block-end with no specific width value, then the browser will set a logical border, that will default to 3px, as that's what the browsers default to for border-block, as a result, the whole element renders at at 39px (1px extra).
So we can either keep the first (initial commit) version, or we completely reset the border using border property instead, and set the border-block-end as before, which includes a defined width for it.
border: 0;
border-block-end: var(--nav-underline-border-width) solid transparent;
I've mocked up a codepen with the initial commit, the 2 line alternative, and your example to show the possible solutions, and why border-width doesn't work in this case: https://codepen.io/editor/pricop/pen/019cde14-71b3-7cc1-be57-d67ead2c0268
Let me know which route to go 👍.
There was a problem hiding this comment.
Ohhh right right. Ya use border: 0 for now—it's a logical reset for the modifier. Good eyes!
scss/_nav.scss
Outdated
There was a problem hiding this comment.
I must've missed this earlier, mind including here?
| --nav-pills-link-active-color: var(--primary-contrast), | |
| --nav-pills-link-active-bg: var(--primary-bg), |
There was a problem hiding this comment.
I've unresolved this temporarily. Please don't push this code just yet, I want to have a deeper look at this.
The --nav-pills-link-active-color applies correctly to nav pills, and it looks fine there, however it doesn't work on navbar pills, which makes use of color: var(--navbar-active-color); instead, due to precedence or specificity.
When active, the text should have been white here instead.
Will update this thread shortly.
There was a problem hiding this comment.
@mdo Since v6 will introduces soft button variants, in my opinion this would be the perfect candidate to use the soft variant colors from buttons both for nav and navbar
What's your take on something like this instead. We could either go with the Primary, or Secondary route.
What do you think?
L.E.: This is already achieved with nav links, my bad.
There was a problem hiding this comment.
It seems that the nav-links already achieve this, my bad 😅.
So I guess pills do make sense to be a solid in that case. I went ahead and commited everything.
Kindly please review whenever you have time.
Will submit a separate PR for the navbar with fixed paddings, and fixed text color for the pills. 👍
There was a problem hiding this comment.
My bad @mdo, I've edited my comment. This is already achieved using the nav-link items.
So pills do make sense to be primary color here. And, in-fact, I've seen cases where this is actually used.
E.g:
So I've included your commit, and fixed the border to be a one-liner.
One last thing if you don't mind, the --nav-tabs-border-radius doesn't seem to be used anymore, is this safe to remove? If yes, I can do this change as well, and we can close this PR.
I'll do a separate PR for the navbar, because I believe there's several ways we can improve it. 👍
There was a problem hiding this comment.
@mdo It can already be achieved simply by using nav link instead of navbar-nav on the navbar component.
It looks 🔥, but I wonder if this was done on purpose? Previously, in BS4 and BS5 nav links would be just... links, with not background effect on hover/active, they only had text color effects.
If this was on purpose, the pills are kinda redundant.
If it wasn't on purpose, then perhaps we could make pills what nav links are today, and nav-links what they used to be in the past (just styled links).
There was a problem hiding this comment.
Yeah it was on purpose because it seemed a little weak of us to not offer a default component that functioned properly. We did the same with .btn having a functional set of styles across all states. It introduces some redundancy for sure, I thought about removing pills, but I think stylistically we can have default nav use subtle theme variants and pills use higher contrast contrast/bg.
There was a problem hiding this comment.
That makes sense, but then you should consider removing pills beacuse the high contrast can be easily achieved now in two different ways (this is where BS + CSS vars really shine).
I was able to achieve 1:1 to what pills component is doing simply by adding style="--bs-nav-link-active-bg: var(--bs-primary-bg); --bs-nav-link-active-color: var(--bs-primary-contrast);" to the nav-link.
Here's the result:
In fact, I've seen this mentioned multiple times in Bootstrap docs, where it suggests customizing the color by using an inline style.
Furthermore, if someone wants this to be the default state for all navigation menus, then they simply customize the component.
Lastly, but not least, by removing the pills, we lower CSS footprint (rather important since BS is now at 270kb+ from 140kb+ in v4).
There was a problem hiding this comment.
And, additionally, this issue will disappear: #42146 (comment) too, as there's no more CSS precedence taking place.
The Docs for nav and navbar would require slight adjustment (we'll have to remove the pills references).
This is definitely for a separate PR. Think about it, and if you're okay with such a change, you can either do it, or I can send a PR.
| --nav-link-padding-x: .75rem, | ||
| --nav-link-padding-y: .375rem, |
There was a problem hiding this comment.
For these values, as you mentioned, I could see us using the --btn-input-* root tokens, but the name needs an update at that point. I can address that later.
| --nav-link-padding-x: var(--btn-input-padding-x), | |
| --nav-link-padding-y: var(--btn-input-padding-y), |
There was a problem hiding this comment.
Fair enough, perhaps something like --interactive-element-padding-x would be an idea since button, input, pagination, nav are elements you interact with.
Just something to think about. Would be nice being able to customize the nav button spacing alongside with the rest of the buttons, without having to deep dive into component specific CSS.
Either way, BS6 is set to be amazing. Saas maps feel so much nicer after using them for a while, compared to the sass varibles 😍.
Co-authored-by: Mark Otto <markdotto@gmail.com>
Shortened the code for borders by using `border` attribute. Also applied the correct background for pill shaped items.
|
Sidebar is totally fine, throw |
Removed the invisible border so that the sidebar remains as condensed as it was before.
|
I've added For the |
Let's do it in another PR once I merge this :). Thanks! |





Currently,
linkandnav-pillsitems height sit at36px(because they don't have any borders), whiletab, andunderlinesit at38px, which perfectly matches the height ofinputandbuttoncomponents.This PR adds an invisible border for the
linkandnav-pillsitems, so that all 4navvariants share the exact same height, regardless of which is used, which makes it suitable to be used with other components, such asinputorbuttonswithout having discrepancy when it comes to elements height.v6 feedback discussion thread at: https://github.com/orgs/twbs/discussions/42145 - where I've included an example with detailed description.