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

Add .nav-underline modifier class #33126

Merged
merged 5 commits into from
Dec 29, 2022
Merged

Add .nav-underline modifier class #33126

merged 5 commits into from
Dec 29, 2022

Conversation

mdo
Copy link
Member

@mdo mdo commented Feb 16, 2021

Exploring an idea here for adding a .nav-underline variant. Styles are pretty simple, but maybe it's still worth it?

Screen Shot 2021-02-16 at 3 17 32 PM

Preview: https://deploy-preview-33126--twbs-bootstrap.netlify.app/docs/5.0/components/navs-tabs/#underline

@XhmikosR
Copy link
Member

Would it make sense to show the underline on hover/focus too? Also, how about using a transition from left to right using ::before and transform?

Just spitballing here :P

@ffoodd
Copy link
Member

ffoodd commented Feb 19, 2021

I'd go with a pseudo-element too, because it'd allow more fine tuning (position, background, playing with other shapes than line, etc.). BTW I'd consider this should be a basis and not a modifier since active state should not be conveyed with color only (which is the case as of now).

Boosted uses a pseudo-element to draw a line (stuck to the content-box) in .nav .active, thus applies it to every possible nav.

My two cents :D

@mdo
Copy link
Member Author

mdo commented Mar 11, 2021

Screen Shot 2021-03-10 at 8 44 53 PM

Refreshed the design and added hover/focus styles. Not sure about building with a pseudo-element @ffoodd. Feels like that might be a little over-engineered for our default styles?

@ffoodd
Copy link
Member

ffoodd commented Mar 11, 2021

Probably, we'll see whatever issue might come up in the future.

The real ones IMHO regarding borders is altered box sizing, and not sticking to text content if we add horizontal padding; but this does not concern our base styles.

So ship it ☺️

Edit: just saw you dropped padding in that case, while it's not an issue it's different from other navs so we may have people asking for them back; moreover that decreases the hit area for those links so they're a bit harder to click — but also increases spacing between them, so there's less chance to click on the wrong item.

No change to my approval, just needed to mention this!

@mdo mdo marked this pull request as ready for review December 29, 2022 04:33
@mdo mdo requested a review from a team as a code owner December 29, 2022 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants