Navbar Migration #1931

akshaaatt · 2022-03-27T17:59:14Z

Bootstrap imports have been fixed in this PR so that JS can be worked on as well.
The navbar broke after switching from version 3 to 5 and hence the necessary fixes have been made.

MonkeyDo · 2022-04-20T16:36:35Z

Finally coming back to this, thanks for your patience !

This is what I see first thing after compiling everything and opening the landing page locally:
Either I'm doing something wrong, or you are :)
Any ideas? Would you mind trying to run it with a fresh local copy?

The collapsible part doesn't collapse, and everything else looks a bit wrong.

MonkeyDo · 2022-04-20T16:45:03Z

Looks like I would need navbar-expand-md instead of -lg for my size screen to have the navbar full-width.
But regardless of my screen size, the navbar looks broken on smaller sizes.

I would also expect that the goal of this PR is to recreate the current navbar, but there are currently lots of changes here from colors to fonts to spacing.
My main review and advice is to aim for reproducing the existing navbar.

MonkeyDo · 2022-04-20T16:47:18Z

~~A thought occurred regarding the non-collapsing navbar: could it be an issue with the JS rather than CSS/HTML?~~

Edit: no I don't think so, looking at the BS docs, it looks like it's the collapse css property that isn't defined properly

MonkeyDo · 2022-04-20T16:47:47Z

(Sorry, wrong button !)

MonkeyDo

Nice to see it all work!
Thanks for working on this, it's not an easy cycle you've started, good work so far :)

I have a few items of feedback, happy to discuss with you at your convenience:

Navbar dropdowns
I find the navbar dropdowns too narrow, which results in a lot of unnecessarily wrapping text:

Modifying the .dropdown-menu class to have a wider min-width (for example 13rem instead of 10rem) or even using width: max-content; gives the container a bit more width to avoid wrapping text, which results in less space on the page:

Considering these changes would have to be done in the design system (IIUC), they don't necessarily need to be done for this PR; I'm sure we will have other css changes to do and it seems somehow wasteful to deploy a new version for a one number change :)

Navbar items hover
Did we decide to use the orange color as hover color for the menu items?
I did like that each project's navbar had a bit of flavor each with its own color.
Similarly, I'm not in love with the gray color of the navbar items' text or the font change, and the hover color should (probably?) be lighter and not darker. Grey/dark gray on orange is not very readable IMO; for comparison here it is with white text:
One last non-navbar related thing: I definitely definitely think the absence of any padding on the page is very jarring and looks like a mistake.

MonkeyDo · 2022-05-19T10:24:07Z

listenbrainz/webserver/static/css/theme/variables.less

-// Navs
-// -------------------------
-
-@nav-link-padding: 10px 15px;
-@nav-link-hover-bg: @gray-lighter;
-
-@nav-disabled-link-color: @gray-light;
-@nav-disabled-link-hover-color: @gray-light;
-
-@nav-open-link-hover-color: @white;
-@nav-open-caret-border-color: #fff;
-
-// Tabs
-@nav-tabs-border-color: #ddd;
-
-@nav-tabs-link-hover-border-color: @gray-lighter;
-
-@nav-tabs-active-link-hover-bg: @body-bg;
-@nav-tabs-active-link-hover-color: @gray;
-@nav-tabs-active-link-hover-border-color: #ddd;
-
-@nav-tabs-justified-link-border-color: #ddd;
-@nav-tabs-justified-active-link-border-color: @body-bg;
-
-// Pills
-@nav-pills-active-link-hover-bg: @component-active-bg;
-@nav-pills-active-link-hover-color: @white;


These don't seem related to the navbar itself (there are other nav elements like tabs etc.).
I think this results in the tabs looking broken (although I'm guessing we'll also replace those in another PR I gather):

listenbrainz/webserver/templates/navbar.html

MonkeyDo · 2022-05-19T11:02:43Z

Ah, found another couple of issues:

the navbar doesn't seem to collapse in small sizes. the hamburger button doesn't do anything.
in screen sizes between collapsed navbar and full width, the navbar doesn't shrink and some items are hidden behind a horizontal scroll:

Bootstrap imports corrects and navbar.html fixed Fix navbar implementstaion Update main.less Fix Search bar Fix navbar Fixed dropdowns

akshaaatt · 2022-05-23T13:54:03Z

Ah, found another couple of issues:

the navbar doesn't seem to collapse in small sizes. the hamburger button doesn't do anything.

in screen sizes between collapsed navbar and full width, the navbar doesn't shrink and some items are hidden behind a horizontal scroll:

Thank you! All fixed :)

MonkeyDo

That looks a lot better, thanks!

There is still one comment from the previous review that is still relevant:
I'm not sure we should be removing the nav-tabs-… and nav-pills-… LESS rules.
Navbar one yes, but nav ones are more general…

MonkeyDo · 2022-05-23T15:03:00Z

listenbrainz/webserver/templates/navbar.html

-          <a class="dropdown-toggle" data-toggle="dropdown" href="#">
-             Explore<span class="caret"></span>
+        <li class="nav-item dropdown">
+          <a class="nav-link dropdown-toggle" data-bs-toggle="dropdown">


Fixes cursor pointer style on hover:

Suggested change

<a class="nav-link dropdown-toggle" data-bs-toggle="dropdown">

<a class="nav-link dropdown-toggle" data-bs-toggle="dropdown" role="button">

MonkeyDo · 2022-05-23T15:03:21Z

listenbrainz/webserver/templates/navbar.html

-          <a class="dropdown-toggle" data-toggle="dropdown" href="#">
-             About<span class="caret"></span>
+        <li class="nav-item dropdown">
+          <a class="nav-link dropdown-toggle" data-bs-toggle="dropdown">


Fixes cursor pointer style on hover:

Suggested change

<a class="nav-link dropdown-toggle" data-bs-toggle="dropdown">

<a class="nav-link dropdown-toggle" data-bs-toggle="dropdown" role="button">

akshaaatt · 2022-05-23T15:21:50Z

That looks a lot better, thanks!

There is still one comment from the previous review that is still relevant: I'm not sure we should be removing the nav-tabs-… and nav-pills-… LESS rules. Navbar one yes, but nav ones are more general…

Makes sense! Fixed

akshaaatt requested a review from MonkeyDo March 27, 2022 18:04

MonkeyDo closed this Apr 20, 2022

MonkeyDo reopened this Apr 20, 2022

akshaaatt force-pushed the fix-navbar branch from 529ef1a to 9bd0110 Compare May 18, 2022 15:09

metabrainz deleted a comment from pep8speaks May 18, 2022

akshaaatt force-pushed the fix-navbar branch 2 times, most recently from 4afbc78 to 42cf4dd Compare May 18, 2022 18:25

MonkeyDo reviewed May 19, 2022

View reviewed changes

Update Navbar component

a2070db

Bootstrap imports corrects and navbar.html fixed Fix navbar implementstaion Update main.less Fix Search bar Fix navbar Fixed dropdowns

akshaaatt force-pushed the fix-navbar branch from 42cf4dd to a2070db Compare May 23, 2022 13:53

akshaaatt requested a review from MonkeyDo May 23, 2022 13:54

MonkeyDo requested changes May 23, 2022

View reviewed changes

Bring back css variables for tabs and fix cursor styles

4774799

akshaaatt requested a review from MonkeyDo May 23, 2022 15:21

MonkeyDo approved these changes May 23, 2022

View reviewed changes

akshaaatt merged commit cc861c2 into bootstrap5 May 23, 2022

akshaaatt deleted the fix-navbar branch May 23, 2022 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navbar Migration #1931

Navbar Migration #1931

akshaaatt commented Mar 27, 2022 •

edited

MonkeyDo commented Apr 20, 2022 •

edited

MonkeyDo commented Apr 20, 2022

MonkeyDo commented Apr 20, 2022 •

edited

MonkeyDo commented Apr 20, 2022

MonkeyDo left a comment

MonkeyDo May 19, 2022

MonkeyDo commented May 19, 2022

akshaaatt commented May 23, 2022

MonkeyDo left a comment

MonkeyDo May 23, 2022

akshaaatt May 23, 2022

MonkeyDo May 23, 2022

akshaaatt May 23, 2022

akshaaatt commented May 23, 2022

	<a class="nav-link dropdown-toggle" data-bs-toggle="dropdown">
	<a class="nav-link dropdown-toggle" data-bs-toggle="dropdown" role="button">

Navbar Migration #1931

Navbar Migration #1931

Conversation

akshaaatt commented Mar 27, 2022 • edited

MonkeyDo commented Apr 20, 2022 • edited

MonkeyDo commented Apr 20, 2022

MonkeyDo commented Apr 20, 2022 • edited

MonkeyDo commented Apr 20, 2022

MonkeyDo left a comment

Choose a reason for hiding this comment

MonkeyDo May 19, 2022

Choose a reason for hiding this comment

MonkeyDo commented May 19, 2022

akshaaatt commented May 23, 2022

MonkeyDo left a comment

Choose a reason for hiding this comment

MonkeyDo May 23, 2022

Choose a reason for hiding this comment

akshaaatt May 23, 2022

Choose a reason for hiding this comment

MonkeyDo May 23, 2022

Choose a reason for hiding this comment

akshaaatt May 23, 2022

Choose a reason for hiding this comment

akshaaatt commented May 23, 2022

akshaaatt commented Mar 27, 2022 •

edited

MonkeyDo commented Apr 20, 2022 •

edited

MonkeyDo commented Apr 20, 2022 •

edited