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

Fix overflowing navbar without max-height. #1

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

jsha
Copy link

@jsha jsha commented Apr 13, 2022

This follows up my proposed changes in rust-lang#1719 (review).

Rather than put height and max-height properties on various things, we put a single height property on the on textual element that is overflowing, and calculate it the same way as the overall navbar height. This looks good on Chrome and Firefox in my local testing, but please try it yourself and let me know what you see.

I think this is somewhat tidier than the original approach.

There's one slight visual problem: since I moved border-left onto div.nav-container .pure-menu-item > .pure-menu-link:first-child, it's also applying to links inside the dropdown menus. So the dropdown menus wind up having 2px of border on the left instead of 1. But I think this can be fixed with some small tweaking.

@GuillaumeGomez
Copy link
Owner

Your changes are almost all good. So the current issues are:

The "double" border (as you mentioned already):

Screenshot from 2022-04-13 20-17-43

When the minimum font size is bigger and you reduce the width of the window, a scrollbar appears (the one I fixed with overflow-x: hidden on the search input):

Screenshot from 2022-04-13 20-16-02

And the input is not at the right size (but that's easy to fix):

Screenshot from 2022-04-13 20-14-34

@jsha
Copy link
Author

jsha commented Apr 13, 2022

When the minimum font size is bigger and you reduce the width of the window, a scrollbar appears (the one I fixed with overflow-x: hidden on the search input)

I was able to reproduce this on docs.rs today, so I don't think it's an issue specific to this PR.

The problem is that #search-input-nav is max-width: 150px. But the <input id="nav-search"> has its width determined by default browser styles. If that's bigger than 150px, the <input> will overflow the <div id="search-input-nav"> and therefore also overflow the flexbox and the nav-container, causing a horizontal scroll bar.

The fix is to remove max-width: 150px on #search-input-nav, and let the div's size be determined by its elements plus the flexbox.

@GuillaumeGomez
Copy link
Owner

Still not working (I think it'll be a nightmare to "correctly" handle this one without the overflow-x):

Screenshot from 2022-04-13 21-31-46

@jsha
Copy link
Author

jsha commented Apr 13, 2022

I think the real issue here is there's too much stuff in the navbar to fit horizontally. We normally fix that by switching to icons when the screen gets narrow enough. The "narrow enough" cutoff is probably incorrect for the amount of stuff we have, particularly when the font size is big. Also, a long crate name might blow away our "narrow enough" cutoff anyhow.

// Pure compatible media queries
// usage:
// @media #{$media-sm} { ... }
$media-sm: "screen and (min-width: 35.5em)";
$media-md: "screen and (min-width: 48em)";
$media-lg: "screen and (min-width: 64em)";
$media-xl: "screen and (min-width: 80em)";

Looks like the media queries are at least specified in em, which should theoretically be responsive to font size. Though I wonder which font size that applies to, and if it takes into account "min font size" set by Firefox.

@GuillaumeGomez
Copy link
Owner

Unfortunately, CSS doesn't have something like "if all items cannot be put on the same line, hide the titles". It's so frustrating... This is why I picked the overflow-x: hidden. It's cheating but at least it worked. 😆

@jsha
Copy link
Author

jsha commented Apr 13, 2022

Right, but the issue you are seeing (horizontal scrollbar when the font size is too big) isn't just a problem of the search box. That's just the first offender. If I turn the minimum font size even higher in Firefox, more of the navbar overflows:

image

I think in that situation, if we can't auto-convert to icons, we should have a horizontal scrollbar. It's ugly, but it fills the need to actually see the whole navbar.

At any rate, I think solving the horizontal scrollbar issue shouldn't be a blocker for rust-lang#1719 (review) because it's not part of the recent regression (and a proper fix will be moderately challenging to find).

@GuillaumeGomez
Copy link
Owner

GuillaumeGomez commented Apr 13, 2022

Not in my PR at least. I think the text overflowing might come from your changes.

EDIT: screenshot with my original PR:

Screenshot from 2022-04-13 22-16-57

Well, the issue is different. It's broken in a different way. XD

@jsha
Copy link
Author

jsha commented Apr 13, 2022

It's the same issue. Notice that the "Rust" menu has completely disappeared in your screenshot.

@GuillaumeGomez
Copy link
Owner

I didn't even notice. 🤣

@GuillaumeGomez
Copy link
Owner

So what do we do about these issues? Do we overlook them since this is already an improvement?

@jsha
Copy link
Author

jsha commented Apr 13, 2022

The "double" border (as you mentioned already):

This is new, we should try and fix before landing.

When the minimum font size is bigger and you reduce the width of the window, a scrollbar appears (the one I fixed with overflow-x: hidden on the search input):

This happens a little less after e3220ff, but we should say this is okay for now and do a more complete fix later.

And the input is not at the right size (but that's easy to fix):

I'll try and fix this next.

@GuillaumeGomez
Copy link
Owner

Thanks! Once done, I'll merge into my PR and I guess we'll be good to go! :)

display: flex;
flex-direction: row;

#search-input-nav {
display: none;
border-left: 1px solid var(--color-border);
height: 100%;
height: calc(#{$top-navbar-height} - 1px);
Copy link
Owner

Choose a reason for hiding this comment

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

It's what I originally wanted to remove by using height: 100% and position: relative. Is there a "best way" between the two maybe?

Copy link
Author

Choose a reason for hiding this comment

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

hm, yeah, maybe height: 100% is the right thing after all. Lemme go back to the drawing board on this one.

Copy link
Owner

Choose a reason for hiding this comment

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

Just to be clear: this is fine for me. It's just that I thought using height: 100% was the good practice. If it's not then it's completely fine. :)

@jsha
Copy link
Author

jsha commented Apr 13, 2022

By the way I found another problem with your original PR: if you scroll the page, the topbar goes away, instead of staying in place. I think that's because of the "position: relative".

@GuillaumeGomez
Copy link
Owner

Arf, it's a pretty bad one. I can't wait for the GUI tests to finally be added... Great catch in any case!

@jsha
Copy link
Author

jsha commented Apr 13, 2022

Okay, I think this version fixes things pretty elegantly. It removes the position: relative that was overriding position: sticky. And it replaces the max-height (which was causing scrollbars on Chrome) with a height: 100%. And it makes sure the height: 100% chain conveys all the way down through .pure-menu-item and .pure-menu-item a. It looks right and scrolls right on both FF and Chrome.

I removed the padding: 8px because that was making the text overflow even if it was slightly big. 8px + 8px + 16px font size == exactly 32px. Now we fall back on the pure-menu rules of padding: 0.5em 1em (i.e. 0.5em top and bottom). At our font size of 0.8em (12.8px), this works out to exactly 31px, which is the space we actually have available after subbing out the bottom border. Text can still visually overflow if the minimum font size is set, but at least it doesn't interfere with the flow of the rustdoc content below.

@GuillaumeGomez
Copy link
Owner

GuillaumeGomez commented Apr 14, 2022

The problem with padding expressed in em is that depending on the size of the font, it changes the padding (which is why I switched to px).

@jsha
Copy link
Author

jsha commented Apr 14, 2022 via email

@GuillaumeGomez
Copy link
Owner

Because you see less of the text. It's bit weird to handle it like this whereas the size of the navbar is expressed in px.

@jsha
Copy link
Author

jsha commented Apr 14, 2022

Okay, I've updated the padding to be expressed in px. I also updated the font sizes to be in px to match, which we should have done anyhow. (12.8 * 1.5) + 6.4 + 6.4 == 32px. 1.5x is the line height.

@GuillaumeGomez
Copy link
Owner

Thanks a lot! I think we fixed all issues so let's merge it then!

@GuillaumeGomez GuillaumeGomez merged commit 7b00973 into GuillaumeGomez:top-navbar Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants