Skip to content

Warning header creates overlap issue with chapter nav #656

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

Closed
adamvoss opened this issue Apr 29, 2017 · 6 comments
Closed

Warning header creates overlap issue with chapter nav #656

adamvoss opened this issue Apr 29, 2017 · 6 comments

Comments

@adamvoss
Copy link
Contributor

Steps to Reproduce:

  1. Go to http://rust-lang.github.io/book/second-edition/ch05-00-structs.html (or any other page that is not the first page or last page.
  2. Click near but not directly on any of the the "collapse nav", "theme, or "print" icons.
  3. Note that you just activated chapter navigation.

I initially thought that the right combination of collapsing the nave and showing the theme drop-down would cause the nav to actually be above the other controls, but I really don't know if that is just the case or if my click was just slightly off.

Browser: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
mdbook: mdbook v0.0.22-pre (435682e)

@carols10cents
Copy link
Member

carols10cents commented May 2, 2017

Aaaaaaaahhhhhh yep. The warning header is pushing down the #menu-bar but not the .nav-chapters.

That is, the .nav-chapters previous link for example is currently here:

screen shot 2017-05-02 at 9 53 03 am

And its top shouldn't be until below the #menu-bar, which it is if I set p.warning to display: none:

screen shot 2017-05-02 at 9 58 37 am

I am slow at CSS and would love help on this issue from anyone reading this!

@jnf
Copy link
Contributor

jnf commented May 2, 2017

Looks like the root cause is in book.css, line 213. It assumes that the menu bar will always be 50px high, which is true. There's also an implicit assumption that the menu bar is always at the top of the page (and thus the only element for which we need clearance), which isn't true.

There's a lot of absolute positioning happening, which makes a fix a bit trickier. Absolute positioning pulls elements out of page flow, so there's no natural adjusting of rendered position based on adjacent elements.

A quick-fix bandaid would be to add a class to .page-wrapper when a warning is going to be included inside <header> that would up the value of top for .nav-chapters to 120px (menu bar + header + header top margin).

.page-wrapper.has-warning > .nav-chapters {
  top: 120px; /* add height for warning content & margin */
} 

Time permitting, I'll send a PR with this fix. It is only a bandaid though. I gotta think about what a more resilient fix would look like.

@steveklabnik
Copy link
Member

steveklabnik commented May 2, 2017 via email

@mikehenrty
Copy link

Well if you don't mind a band-aid, I have a quick "fix" that is hackier than @jnf's suggestion but might have a lower footprint. I'll send a PR and let you folks decide.

mikehenrty added a commit to mikehenrty/book that referenced this issue May 2, 2017
jnf added a commit to jnf/book that referenced this issue May 2, 2017
The status class, `.has-warning`, provides a hook for other content to adjust
their positioning. It's leveraged to push down the absolutely positioned chapter
navs far enough so that they clear both the warning message and the icons in the
header bar. Fixes rust-lang#656.
@jnf
Copy link
Contributor

jnf commented May 2, 2017

The only wiggly bit with a z-index fix (#668) is that it turns the "dead space" between and surrounding the sidebar and theme toggles into a clickable hot zone. The chapter nav is positioned just below, so missing the icon by a pixel will navigate users away from the page. Not a huge deal, but potentially frustrating to readers.

@mikehenrty
Copy link

If @jnf's PR works in all cases (ie. it doesn't move down the chapter navigation when the warning is not displayed), then we should use his because it is a better solution 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants