Fix top navbar style #1719

GuillaumeGomez · 2022-04-11T13:22:59Z

I just realized that the display of the top navbar menu is broken (well, not good looking):

It comes from my changes in #1717 to allow having a different font size.

Overall, the style for the top navbar is quite the nightmare so I decided to remove the float properties and to use flex instead. The result is quite nice (imo) and fixes some small UI issues:

And it looks like this with bigger font size:

I checked other pages and they work well too.

jsha · 2022-04-12T22:16:59Z

In my local Chrome, with the light theme, it looks like this:

There are grey rectangles between the menu names. Looking the Dev Tools, it appears to be due to these rules in the light CSS:

::-webkit-scrollbar-thumb {
    background-color: rgba(36,37,39,0.6);
}
::-webkit-scrollbar-track {
    background-color: #ecebeb;
}

Those are getting applied to the <div class="pure-menu pure-menu-horizontal">, but I'm not sure why.

GuillaumeGomez · 2022-04-12T22:21:23Z

Great catch! I'll check tomorrow what's going on.

jsha · 2022-04-12T22:38:20Z

templates/style/_navbar.scss

+            max-height: 100%;
+            overflow-x: hidden;


I think these lines are unnecessary. When I remove these properties locally, the unwanted grey rectangles (which are tiny vertical scrollbars) go away. Also, with these properties removed, even if the menu items' font size is weird, it doesn't interfere with the main page.

In fact, I think this whole CSS rule (including the comment about what happens with a locally installed old Fira Sans) should get deleted. Switching to the flex layout seems like it sufficed to fix the issue with overflowing menu titles, even without this rule.

(I just tested in a locally installed Firefox, with the Fira Sans from #1669 (comment), at these commits:

ad8d4fe (i.e. right before Set overflow-y: hidden on navbar #1711)

0ae5807 (this branch)

0ae5807 with this CSS rule removed

And I can confirm that at (1) I have the overflow issue. Both (2) and (3) fix the overflow issue, and (3) additionally fixes the scrollbar issue introduced in this branch.

BTW the scrollbar issue seems to be Chrome-only.

Without this change:

With this change:

So we definitely need it.

jsha · 2022-04-12T23:28:15Z

templates/style/_navbar.scss

+    position: relative;
+
+    .container, .pure-menu-horizontal {
+        position: relative;
+        height: 100%;


What are the position: relative and height: 100% needed for here? I think they are unnecessary.

It's because with this, we can set the element of the navbar to height: 100%. It prevents the bottom margin to disappear in case the font size is too big.

jsha · 2022-04-12T23:29:02Z

templates/style/_navbar.scss

    form.landing-search-form-nav {
        max-width: 1200px;
+        height: 100%;


I think you can get rid of this height: 100%. That makes the search field vertically misaligned, but you can fix that by also removing the height: 100% on line 92.

Without the height: 100%:

jsha · 2022-04-12T23:29:20Z

templates/style/_navbar.scss

            max-width: 150px;
            display: none;
            border-left: 1px solid var(--color-border);
+            height: 100%;
+            overflow-x: hidden;


Why do we need overflow-x: hidden here?

It's a trick I used to fix the scrollbar issue when the font-size is too big. I thought flex-shrink: 1 would do it but apparently it doesn't...

jsha · 2022-04-12T23:35:33Z

templates/style/_navbar.scss

+       }
+
+       .pure-menu-item {
+            height: 100%;


I think this is also unnecessary.

jsha · 2022-04-12T23:36:14Z

Overall, great change! I am really happy to have the items show up in the same HTML order as the visual order on the page.

GuillaumeGomez · 2022-04-13T15:00:53Z

In my local Chrome, with the light theme, it looks like this:

There are grey rectangles between the menu names. Looking the Dev Tools, it appears to be due to these rules in the light CSS:
::-webkit-scrollbar-thumb {
    background-color: rgba(36,37,39,0.6);
}
::-webkit-scrollbar-track {
    background-color: #ecebeb;
}
Those are getting applied to the <div class="pure-menu pure-menu-horizontal">, but I'm not sure why.

I can't reproduce it on my chrome:

Any idea what's going on in yours? Do you have some extension or some other custom setting maybe?

jsha · 2022-04-13T15:45:22Z

Mine is Chrome Version 100.0.4896.88 (Official Build) (64-bit) on Ubuntu 21.10. It still reproduces when I run google-chrome --user-data-dir=$(mktemp -d) http://localhost:3000/regex/latest/regex/ to have a totally clean profile.

I do have that old copy of Fira Sans still installed, which probably contributes.

If I remove the max-height: 100%, the natural height of those menu titles is 36px. The max-height constrains their parent element, which leads to a scrollbar.

If you follow the tweaks I recommended above that will fix things.

GuillaumeGomez · 2022-04-13T15:47:38Z

I tested all of them and they all ended up in bad rendering in firefox. :-/

EDIT: I forgot to precise: bad rendering when the minimum font size is more than expected.

jsha · 2022-04-13T16:41:01Z

Which type of bad rendering? The "body text is too narrow" bad rendering? Or "bottom border of menu title is not visible"?

GuillaumeGomez · 2022-04-13T17:11:36Z

Size of top navbar items becomes chaotic:

For example, sysinfo item even gets its height different from the rest.

jsha · 2022-04-13T17:47:30Z

I spent some time fiddling with this locally and came up with a solution that I think is tidier, avoids the scrollbars on Chrome, and looks right on both Chrome and Firefox. I sent it as a PR against your branch: GuillaumeGomez#1. Let me know what you think!

GuillaumeGomez · 2022-04-13T17:48:52Z

Nice, thanks a lot! Will test it locally as soon as possible.

Fix overflowing navbar without max-height.

GuillaumeGomez · 2022-04-14T17:34:47Z

I integrated @jsha's improvements so this should now be ready!

jsha · 2022-04-14T18:36:38Z

I've manually tested the version on this branch in Firefox and Chrome, with the old version of Fira Sans locally installed. Tests I did:

Scrolled vertically to make sure the navbar stayed in place
Opened and closed menus
Verified that the rustdoc content was not artificially narrow
Verified that the vertical bars separating menu titles went all the way to the top and bottom of the navbar
Verified that menu titles were approximately centered vertically

I also tested with a large font size in firefox, and a large minimum font size. The presentation wasn't perfect but it was good enough. And it didn't interere with the rustdoc content.

syphar

LGTM, definitely fixes #1725

Fix top navbar style

0ae5807

github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Apr 11, 2022

jsha requested changes Apr 12, 2022

View reviewed changes

syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Apr 13, 2022

jsha mentioned this pull request Apr 13, 2022

Fix overflowing navbar without max-height. GuillaumeGomez/docs.rs#1

Merged

jsha and others added 3 commits April 13, 2022 15:22

Fix up top-navbar tweaks

0469099

Switch font sizes to px

6f9815c

Merge pull request #1 from jsha/top-navbar2

7b00973

Fix overflowing navbar without max-height.

syphar mentioned this pull request Apr 16, 2022

Menus are cramped, dependencies wrap badly #1725

Closed

syphar added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Apr 16, 2022

syphar approved these changes Apr 16, 2022

View reviewed changes

syphar merged commit de197f9 into rust-lang:master Apr 16, 2022

github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Apr 16, 2022

syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Apr 16, 2022

GuillaumeGomez deleted the top-navbar branch April 16, 2022 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix top navbar style #1719

Fix top navbar style #1719

GuillaumeGomez commented Apr 11, 2022

jsha commented Apr 12, 2022

GuillaumeGomez commented Apr 12, 2022

jsha Apr 12, 2022

jsha Apr 12, 2022

jsha Apr 12, 2022

GuillaumeGomez Apr 13, 2022 •

edited

Loading

jsha Apr 12, 2022

GuillaumeGomez Apr 13, 2022

jsha Apr 12, 2022

GuillaumeGomez Apr 13, 2022

jsha Apr 12, 2022

GuillaumeGomez Apr 13, 2022

jsha Apr 12, 2022

jsha commented Apr 12, 2022

GuillaumeGomez commented Apr 13, 2022

jsha commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022 •

edited

Loading

jsha commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022

jsha commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022

GuillaumeGomez commented Apr 14, 2022

jsha commented Apr 14, 2022

syphar left a comment

Fix top navbar style #1719

Fix top navbar style #1719

Conversation

GuillaumeGomez commented Apr 11, 2022

jsha commented Apr 12, 2022

GuillaumeGomez commented Apr 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha commented Apr 12, 2022

GuillaumeGomez commented Apr 13, 2022

jsha commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022 • edited Loading

jsha commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022

jsha commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022

GuillaumeGomez commented Apr 14, 2022

jsha commented Apr 14, 2022

syphar left a comment

Choose a reason for hiding this comment

GuillaumeGomez Apr 13, 2022 •

edited

Loading

GuillaumeGomez commented Apr 13, 2022 •

edited

Loading