use <body> instead of separate wrapper div for navbar-fixed #4852

Snewk · 2017-06-20T22:59:54Z

Description

The materialize navbar handles a fixed navigation bar by creating a fixed wrapper div underneath the navbar. The extra div handles the page offset(via the height property) and z-index.

This commit moves the z-index property to the actual .navbar-fixed nav element itself, and handles offset using topon both elements

Repro Steps

1: create a <nav> element.
2: add class="navbar-fixed" to the parent element (<body>).
3: scroll down to see elements on top of the fixed navbar instead of underneath it.
4: page offset does not function in this case.

Screenshots / Codepen

https://codepen.io/anon/pen/vZmWEy

Corrected

https://codepen.io/anon/pen/Kqmypx

NOTE: this is a correction on a previous PR for the same issue.

The text was updated successfully, but these errors were encountered:

DanielRuf · 2017-06-21T04:21:30Z

@Snewk did you try to add a wrapper div around the nav and give it the navbar-fixed class? Because this creates a separate block for the nav and sets the position for it.

https://codepen.io/anon/pen/xrdjbq

Generally your PR is good and moving the class to the body to toggle it easily is a good idea and an actual use case.

https://codepen.io/anon/pen/eRWrNZ

Can you update the docs (in the jade/page_contents directory) and change the paragraph in navbar.html, remove the wrapper div and update the wording to point at body.

Thanks for the PR. This should make the component more flexible.

Snewk · 2017-06-21T06:41:42Z

@DanielRuf When testing, I tried using <body>as the wrapper div and giving it the navbar-fixed class directly(as opposed to creating a separate wrapping div). This created a fixed navbar, however using <body classs="navbar-fixed"> created two other issues.

The first was the z-index issue which i attempted to fix with my PR, but I missed the second issue until I read your comment.

In my example, there is no extra div between <body> and <nav> so the navbar ends up covering the top of the page content.

Also, in this case, is no way to offset the page content to a custom value. In fact, trying to adjust the height will increase the height of the body from the bottom instead of the top.

I suggest leaving the navbar-fixed class as-is and closing this issue and PR. I probably should have read the documentation more carefully before submitting this as a bug.

DanielRuf · 2017-06-21T14:47:41Z

@Snewk your PR makes sense for special use cases.

@tomscholz should we enforce our rules or give developers more freedom over this?

DanielRuf · 2017-06-21T19:42:07Z

Also, in this case, is no way to offset the page content to a custom value. In fact, trying to adjust the height will increase the height of the body from the bottom instead of the top.

@Snewk Do you mean the $navbar-mobile-height variable or how did you do this?

Snewk · 2017-06-21T20:06:28Z

Also, in this case, is no way to offset the page content to a custom value. In fact, trying to adjust the height will increase the height of the body from the bottom instead of the top.

@DanielRuf Yes, i meant by changing the height directly or by changing $navbar-height-mobile. Take a look at the following structure:
<body>
        <div class="navbar-fixed"
                <nav></nav>
        </div>
</body>

Here, the div gets z-index: 997; as well as height: $navbar-height-mobile;. This div holds the navbar over the page content, and it pushes the page down by $navbar-height-mobile. Everything works.

BUT, if we use the following:
<body class="navbar-fixed">
<nav></nav>
</body>

The z-index and height cannot be assigned to the body in the same way. There is no element underneath the navbar to hold it above the content at z-index: 997; or to offset the page by $navbar-height-mobile.

If we want to allow developers the freedom to use <body class="navbar-fixed">, we should incorporate my PR. Unfortunately, this will still leave the issue of page content being hidden underneath the navbar.

I would be happy to update the documentation to reflect this limitation if you decide to implement my change.

Snewk · 2017-06-21T20:28:38Z

After some more tinkering, I may have come up with a solution.
https://codepen.io/anon/pen/Kqmypx

instead of giving the navbar-fixed wrapper a height: $navbar-height-mobile, we can use top: $navbar-height-mobile instead and then add top: 0px to the nav element.

DanielRuf · 2017-06-21T20:45:34Z

@Snewk ah right. I should have tried the different offsetting solutions for fixed elements.

Cool. There is always a solution ;-)

Please also make the changes in the branch of your PR.

Snewk · 2017-06-21T20:59:27Z

glad to help. here is the PR
#4859

DanielRuf · 2017-06-21T21:04:40Z

Looks much better.

@acburst @tomscholz your opinions?

Snewk · 2017-06-29T21:57:54Z

@DanielRuf @tomscholz

Found an issue with my PR so I closed it. Fixed it in a new PR. Here is the commit :
15b5ff4

DanielRuf · 2017-06-29T21:59:42Z

@Snewk you can push new commits to your (old) branch to update your PR.

Snewk · 2017-06-29T22:00:55Z

my bad. i am still pretty new to contributing to projects i use. will make sure to do that in the future.

Snewk · 2017-06-30T04:04:25Z

@DanielRuf i agree that we need to test this change to ensure things continue working as expected.

I noticed the file test/html/fixed_navbar.html is this currently in use anywhere? reading the docs and code, it looks like jasmine only uses the tests/ directory and not the test/ dir.

let me know if i can help testing or with writing a test for this component.

gusbemacbe · 2017-07-20T05:07:06Z

Then in the version 0.100, unfortunately navbar-fixed still suffered a bug.

DanielRuf · 2017-07-20T05:37:01Z

Then in the version 0.100, unfortunately navbar-fixed still suffered a bug.

Which bug? Please be more precise, create a new issue and provide a new codepen.

gusbemacbe · 2017-07-20T05:47:35Z

Check my issue: #4986, @DanielRuf.

pvanheus · 2017-10-28T05:36:41Z

@Snewk your description of the problem "the navbar ends up covering the top of the page content" is exactly what I'm seeing (see codepen https://codepen.io/pvanheus/pen/GORrJX adapted from the one you posted). Is 15b5ff4 meant to solve this problem?

Dogfalo · 2018-03-01T23:44:38Z

Move the sidenav ul out of the navbar as described in #3844

Snewk changed the title ~~Fixed nav does not have z-index~~ eliminate separate div for navbar-fixed wrapper Jun 21, 2017

Snewk changed the title ~~eliminate separate div for navbar-fixed wrapper~~ use <body> instead of separate wrapper div for navbar-fixed Jun 26, 2017

tomscholz added the has-pr label Jul 4, 2017

Dogfalo closed this as completed Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use <body> instead of separate wrapper div for navbar-fixed #4852

use <body> instead of separate wrapper div for navbar-fixed #4852

Snewk commented Jun 20, 2017 •

edited

Loading

DanielRuf commented Jun 21, 2017 •

edited

Loading

Snewk commented Jun 21, 2017 •

edited

Loading

DanielRuf commented Jun 21, 2017 •

edited

Loading

DanielRuf commented Jun 21, 2017

Snewk commented Jun 21, 2017

Snewk commented Jun 21, 2017

DanielRuf commented Jun 21, 2017 •

edited

Loading

Snewk commented Jun 21, 2017 •

edited

Loading

DanielRuf commented Jun 21, 2017

Snewk commented Jun 29, 2017 •

edited

Loading

DanielRuf commented Jun 29, 2017 •

edited

Loading

Snewk commented Jun 29, 2017

Snewk commented Jun 30, 2017

gusbemacbe commented Jul 20, 2017

DanielRuf commented Jul 20, 2017

gusbemacbe commented Jul 20, 2017

pvanheus commented Oct 28, 2017

Dogfalo commented Mar 1, 2018

use <body> instead of separate wrapper div for navbar-fixed #4852

use <body> instead of separate wrapper div for navbar-fixed #4852

Comments

Snewk commented Jun 20, 2017 • edited Loading

Description

Repro Steps

Screenshots / Codepen

Corrected

DanielRuf commented Jun 21, 2017 • edited Loading

Snewk commented Jun 21, 2017 • edited Loading

DanielRuf commented Jun 21, 2017 • edited Loading

DanielRuf commented Jun 21, 2017

Snewk commented Jun 21, 2017

Snewk commented Jun 21, 2017

DanielRuf commented Jun 21, 2017 • edited Loading

Snewk commented Jun 21, 2017 • edited Loading

DanielRuf commented Jun 21, 2017

Snewk commented Jun 29, 2017 • edited Loading

DanielRuf commented Jun 29, 2017 • edited Loading

Snewk commented Jun 29, 2017

Snewk commented Jun 30, 2017

gusbemacbe commented Jul 20, 2017

DanielRuf commented Jul 20, 2017

gusbemacbe commented Jul 20, 2017

pvanheus commented Oct 28, 2017

Dogfalo commented Mar 1, 2018

Snewk commented Jun 20, 2017 •

edited

Loading

DanielRuf commented Jun 21, 2017 •

edited

Loading

Snewk commented Jun 21, 2017 •

edited

Loading

DanielRuf commented Jun 21, 2017 •

edited

Loading

DanielRuf commented Jun 21, 2017 •

edited

Loading

Snewk commented Jun 21, 2017 •

edited

Loading

Snewk commented Jun 29, 2017 •

edited

Loading

DanielRuf commented Jun 29, 2017 •

edited

Loading