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

Offcanvas navbar #34273

Merged
merged 1 commit into from
Jul 18, 2021
Merged

Offcanvas navbar #34273

merged 1 commit into from
Jul 18, 2021

Conversation

mdo
Copy link
Member

@mdo mdo commented Jun 16, 2021

This is a replacement for #33537. Fixes #34098.

The original PR gives us a great starting point, and this PR builds on that by turning the static styles into source Sass that's generated by our breakpoints and is an official component modifier. The way this is built, the new .navbar-offcanvas-* classes depend on .navbar-expand-* classes, so you'll always need a pairing of the two. I think this is okay so as to avoid further duplication.

One thing that I'm unsure about is the .navbar-offcanvas.navbar-expand situation, which basically says "don't ever use an offcanvas. I think we could omit the xs .navbar-offcanvas class entirely, but that might cause issues for others. Until then, it's kind of just useless as this combination will never show a button to open the offcanvas.

/cc @twbs/css-review @craftwerkberlin

Preview: https://deploy-preview-34273--twbs-bootstrap.netlify.app/docs/5.0/components/navbar/#offcanvas

@crftwrk
Copy link
Contributor

crftwrk commented Jun 16, 2021

Great! Sass file from example is here

https://github.com/craftwerkberlin/bootstrap-offcanvas-navbar/blob/main/assets/dist/css/_navbar-offcanvas.scss

@mdo there is no .navbar-offcanvas anymore, just use .navbar-expand-* classes

@mdo
Copy link
Member Author

mdo commented Jun 23, 2021

@craftwerkberlin Nice, just pushed changes up to reflect that :). Should be getting close here!

@mdo mdo marked this pull request as ready for review June 23, 2021 23:44
@mdo mdo requested a review from a team as a code owner June 23, 2021 23:44
@crftwrk
Copy link
Contributor

crftwrk commented Jun 28, 2021

@mdo I would kindly ask if you can have a glance again to the Sass file linked in my previous comment.

There is a second part of implementation starting in line 53. With this you can override navbar-{light | dark} below the breakpoints by adding offcanvas-{dark | light} class to offcanvas.

Why this is important? When you check my example again https://examples.bootscore.me/bootstrap-offcanvas-navbar/, the first fixed-top examples uses navbar-dark bg-dark to show nav-link in high contrast on expanded navbar. But collapsed offcanvas uses default white background. In this case you cannot see any nav-links, because they are white on white background. In example class offcanvas-light is added to show dark nav-links on white background in collapsed offcanvas.

I think that is a pretty simple but usefull option to make offcanvas more flexible and hope this will be part of this PR.

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@XhmikosR
Copy link
Member

  1. can we make the navbar always collapsed even on desktop?
  2. there's a page jump when the navbar button is clicked but doesn't happen when I use browser's dev tools to emulate a device (so only happens if I manually resize my browser to show the offcanvas button and click on it, browser Firefox)

@mdo
Copy link
Member Author

mdo commented Jul 7, 2021

  1. can we make the navbar always collapsed even on desktop?

That's already permissible, it's the code snippet below the example.

  1. there's a page jump when the navbar button is clicked but doesn't happen when I use browser's dev tools to emulate a device (so only happens if I manually resize my browser to show the offcanvas button and click on it, browser Firefox)

Hmm, not seeing that on my phone.

@XhmikosR
Copy link
Member

XhmikosR commented Jul 9, 2021

That's already permissible, it's the code snippet below the example.

I know that's permissible, but I feel like using that instead of what we use now will make it clearer :) Like swapping the snippets. On the other hand, I don't think we do this anywhere else, but I just feel it makes sense to always have it collapsed.

Hmm, not seeing that on my phone.

Doesn't happen using a phone or the developer tools emulation, but does happen if you have manually resized the window (at least for me here, with FF and Edge, Windows 10, 250% DPI)

Co-Authored-By: craftwerk <1193597+craftwerk@users.noreply.github.com>
@mdo mdo merged commit 06a1ca5 into main Jul 18, 2021
@mdo mdo deleted the pr/33537 branch July 18, 2021 03:33
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.

V5 Navbar : Make navbar collapse as offcanvas
5 participants