Navigation: New NavBar designs behind feature toggle #41045

ashharrison90 · 2021-10-28T13:52:40Z

What this PR does / why we need it:

implements the new NavBar designs behind the newNavigation feature toggle. changes:

splits the NavBar into 4 sections. search, core apps, plugin apps and config/settings.
removes the Home link from the dashboard menu and adds it as a standalone item in the NavBar
removes the + button from the NavBar and adds the actions to the dashboard menu
moves Server Admin + Configuration to the config/settings (bottom) section
refactors the grafana logo to be a NavBarItem, same as everything else
created a new generic NavBarSection component and removed the existing TopSection/BottomSection in favour of this
- moved the fairly specific logic of BottomSection into a util function enrichConfigItems
adjusted profile weight so that it appears below config and admin (in practice this was already happening because they were split into BottomSection/TopSection respectively)

with feature toggle off	with feature toggle on

Which issue(s) this PR fixes:

For #37961

Special notes for your reviewer:

i apologise in advance for my go. any pointers to make that nicer would be greatly appreciated 🙏

hugohaggmark · 2021-10-29T04:12:08Z

public/app/core/components/NavBar/NavBar.tsx

+    if (newNavigationEnabled) {
+      return !item.hideFromMenu && !item.id?.startsWith('plugin-page-');
+    }


Is this something we can control in the backend instead?

hugohaggmark

Great work as always, I left some small comments.

I found the Grafana logo to become smaller on this branch when the feature toggle is off.

Before

After

I know that this is just the beginning but so much negative space when you don't have app plugins 😞

hugohaggmark · 2021-10-29T04:15:12Z

public/app/core/components/NavBar/NavBarSection.tsx

+  className?: string;
+}
+
+const NavBarSection = ({ children, className }: Props) => {


nit: we prefer function({...} : Props): ReactElement over const

hugohaggmark · 2021-10-29T04:15:57Z

public/app/core/components/NavBar/NavBarSection.tsx

+  );
+};
+
+export default NavBarSection;


nit: we prefer export const NavBarSection over export defaults.

hugohaggmark · 2021-10-29T04:16:32Z

public/app/core/components/NavBar/utils.ts

@@ -9,10 +14,69 @@ export const getForcedLoginUrl = (url: string) => {
  return `${getConfig().appSubUrl}${url.split('?')[0]}?${queryParams.toString()}`;
 };

+export const enrichConfigItems = (


This function seems testable?

ashharrison90 · 2021-10-29T09:22:42Z

@hugohaggmark for the logo, good eyes 👀

now that it's a NavBarItem it's the same size as everything else in the NavBar. do you think it should be bigger (as before), or just pointing out the difference? 🤔

hugohaggmark · 2021-10-29T09:25:03Z

@hugohaggmark for the logo, good eyes 👀

now that it's a NavBarItem it's the same size as everything else in the NavBar. do you think it should be bigger (as before), or just pointing out the difference? 🤔

Because we use a feature toggle, things without the toggle should remain the same.

torkelo · 2021-10-29T09:42:58Z

visually this feels very complex to me all these separate sections and borders (where the navbar is not at the edge but actually lives 8px from the left edge)

ashharrison90 · 2021-10-29T09:52:49Z

mmm that's fair feedback, fyi @lukasztyrala

i think it still needs some usability testing so it's probably not the final design. that's the beauty of the toggle, we can iterate 😄 how do you feel about the functional changes @torkelo?

torkelo · 2021-10-29T09:56:19Z

But I think it sort of works as well (the left padding), but I am not sure separating the sections that much is helpful, we also want app plugins to feels like any other core Grafana feature

lukasztyrala · 2021-10-29T10:04:35Z

We are putting this behind a feature flag to test with people. We want to validate some assumptions and directions. This is not final by any means/dimensions. :-)

…ew sections

…omments

hugohaggmark

Frontend code looks great and thanks for adding tests. Added some comments and would be good if someone in @grafana/backend-platform could review the backend parts too.

hugohaggmark · 2021-11-01T05:57:49Z

pkg/api/index.go

-		Id:           "profile",
-		Img:          gravatarURL,
-		Url:          hs.Cfg.AppSubURL + "/profile",
-		HideFromMenu: true,


nit: HideFromMenu removed here, probably ok?

yeah, now we've added the Section property to the struct, this HideFromMenu becomes redundant and a little confusing (since at the top level it never hid anything from the menu, it was used to control what appeared in TopSection/BottomSection).

So when the feature toggle is the new "default" we need to revisit this and deprecate/remove HideFromMenu.

yeah, we probably need to separate out NavLink and create NavLinkChild or something. for NavLinkChild it makes sense and behaves as expected. for a top level NavLink it was kinda being repurposed

hugohaggmark · 2021-11-01T06:02:37Z

public/app/core/components/NavBar/utils.ts

@@ -23,7 +89,7 @@ export const isLinkActive = (pathname: string, link: NavModelItem) => {
      // alert channel match
      // TODO refactor routes such that we don't need this custom logic
      return true;
-    } else if (linkPathname === '/' && pathname.startsWith('/d/')) {
+    } else if (linkPathname === dashboardLinkMatch && pathname.startsWith('/d/')) {


Any tests that need updating?

there weren't, but that was a little concerning tbh so i've added some more tests 😄

hugohaggmark · 2021-11-01T06:05:19Z

public/app/core/components/NavBar/NavBar.tsx

+
+      {!newNavigationEnabled && (
+        <NavBarSection>
+          <NavBarItem url={homeUrl} label="Home" className={styles.grafanaLogo} showMenu={false}>
+            <Branding.MenuLogo />
+          </NavBarItem>
+          <NavBarItem
+            className={styles.search}
+            isActive={activeItemId === 'search'}
+            label="Search dashboards"
+            onClick={onOpenSearch}
+          >
+            <Icon name="search" size="xl" />
+          </NavBarItem>
+        </NavBarSection>
+      )}
+      {newNavigationEnabled && (
+        <NavBarSection>
+          <NavBarItem url={homeUrl} label="Home" className={styles.grafanaLogo} showMenu={false}>
+            <Branding.MenuLogo />
+          </NavBarItem>
+          <NavBarItem
+            className={styles.search}
+            isActive={activeItemId === 'search'}
+            label="Search dashboards"
+            onClick={onOpenSearch}
+          >
+            <Icon name="search" size="xl" />
+          </NavBarItem>
+        </NavBarSection>
+      )}
+
+      <NavBarSection>
+        {coreItems.map((link, index) => (
+          <NavBarItem
+            key={`${link.id}-${index}`}
+            isActive={activeItemId === link.id}
+            label={link.text}
+            menuItems={link.children}
+            target={link.target}
+            url={link.url}
+          >
+            {link.icon && <Icon name={link.icon as IconName} size="xl" />}
+            {link.img && <img src={link.img} alt={`${link.text} logo`} />}
+          </NavBarItem>
+        ))}
+      </NavBarSection>
+
+      {pluginItems.length > 0 && (
+        <NavBarSection>
+          {pluginItems.map((link, index) => (
+            <NavBarItem
+              key={`${link.id}-${index}`}
+              isActive={activeItemId === link.id}
+              label={link.text}
+              menuItems={link.children}
+              menuSubTitle={newNavigationEnabled ? link.subTitle : undefined}
+              onClick={link.onClick}
+              target={link.target}
+              url={link.url}
+            >
+              {link.icon && <Icon name={link.icon as IconName} size="xl" />}
+              {link.img && <img src={link.img} alt={`${link.text} logo`} />}
+            </NavBarItem>
+          ))}
+        </NavBarSection>
+      )}
+
+      <div className={styles.spacer} />
+
+      <NavBarSection>
+        {configItems.map((link, index) => (
+          <NavBarItem
+            key={`${link.id}-${index}`}
+            isActive={activeItemId === link.id}
+            label={link.text}
+            menuItems={link.children}
+            menuSubTitle={link.subTitle}
+            onClick={link.onClick}
+            reverseMenuDirection
+            target={link.target}
+            url={link.url}
+          >
+            {link.icon && <Icon name={link.icon as IconName} size="xl" />}
+            {link.img && <img src={link.img} alt={`${link.text} logo`} />}
+          </NavBarItem>
+        ))}
+      </NavBarSection>
+
+      {showSwitcherModal && <OrgSwitcher onDismiss={toggleSwitcherModal} />}


nit: I think I would create 2 separate NavBar components one for the current and one for the feature toggle. I think that makes it a lot cleaner.

Agree with this one.

mckn

Looks really great! Exiting stuff.

Question: what is the long-term plan for the navModel? Something we can get rid of/enhance in Grafana 9?

…abled

ashharrison90 · 2021-11-01T11:32:03Z

Question: what is the long-term plan for the navModel? Something we can get rid of/enhance in Grafana 9?

great question @mckn, it's part of what we're trying to tackle with the configurable nav bar proposals that are sort of bubbling in the background right now.

navModel will probably need to stay in some respect. one proposal is we move it from bootData to an api, then we can allow a user to reconfigure their navModel (what items are in the navModel, their order, etc) via UI interactions which will trigger api calls to the backend.

would love any input/suggestions you have on that. think it's fair to say it's the part we're struggling the most on right now.

* Navigation: Remove plus button behind feature toggle * Navigation: Add home button behind feature toggle * Navigation: Move settings/admin to bottom section behind feature toggle * Navigation: Refactor grafana logo to be a NavBarItem * Navigation: Create new PluginSection and styling changes to support new sections * Navigation: Hack to use mobile menu as a mega menu for now * Navigation: Only render plugin section if there are items * Navigation: mobile menu is always 100% width if toggle is off * Navigation: Reset width back to 48 and fix broken css property * Navigation: Create generic NavBarSection component to reduce repetition * Navigation: Don't show sublinks for core items * Navigation: Comments from UX review * Navigation: Remove mobile menu hack * Navigation: Unit tests for enrichConfigItems and other minor review comments * Navigation: Move section logic to backend * Navigation: Refactor alerting links out into a separate function * Navigation: More tests for isLinkActive * Linting... * Navigation: Create new NavBar component for when feature toggle is enabled

ashharrison90 added area/frontend area/navigation labels Oct 28, 2021

ashharrison90 added this to the 8.3.0 milestone Oct 28, 2021

ashharrison90 requested review from a team as code owners October 28, 2021 13:52

ashharrison90 self-assigned this Oct 28, 2021

ashharrison90 requested review from kminehart, ying-jeanne, axelavargas and thisisobate and removed request for a team October 28, 2021 13:52

grafanabot added the area/backend label Oct 28, 2021

ashharrison90 mentioned this pull request Oct 28, 2021

Epic: Navigation improvements (WIP) #37961

Closed

23 tasks

hugohaggmark reviewed Oct 29, 2021

View reviewed changes

hugohaggmark approved these changes Oct 29, 2021

View reviewed changes

ashharrison90 added 7 commits October 29, 2021 13:51

Navigation: Remove plus button behind feature toggle

e246a66

Navigation: Add home button behind feature toggle

f4a796c

Navigation: Move settings/admin to bottom section behind feature toggle

36100a4

Navigation: Refactor grafana logo to be a NavBarItem

1b8ec02

Navigation: Create new PluginSection and styling changes to support n…

08f72da

…ew sections

Navigation: Hack to use mobile menu as a mega menu for now

301475d

Navigation: Only render plugin section if there are items

1246440

ashharrison90 added 6 commits October 29, 2021 13:51

Navigation: mobile menu is always 100% width if toggle is off

e7e15fa

Navigation: Reset width back to 48 and fix broken css property

71d6972

Navigation: Create generic NavBarSection component to reduce repetition

d0e4fd4

Navigation: Don't show sublinks for core items

1848381

Navigation: Comments from UX review

ec2cca7

Navigation: Remove mobile menu hack

1909622

ashharrison90 force-pushed the ash/navbar-enhancements branch from 7f2928e to 1909622 Compare October 29, 2021 14:05

ashharrison90 added 2 commits October 29, 2021 15:05

Navigation: Unit tests for enrichConfigItems and other minor review c…

8e9d6bb

…omments

Navigation: Move section logic to backend

e578c06

ashharrison90 requested a review from a team as a code owner October 29, 2021 15:18

Navigation: Refactor alerting links out into a separate function

ab5b14f

ashharrison90 requested a review from hugohaggmark October 29, 2021 17:55

hugohaggmark approved these changes Nov 1, 2021

View reviewed changes

mckn approved these changes Nov 1, 2021

View reviewed changes

ashharrison90 added 3 commits November 1, 2021 11:03

Navigation: More tests for isLinkActive

82b4cb9

Linting...

d51fbc6

Navigation: Create new NavBar component for when feature toggle is en…

2a9bf65

…abled

ashharrison90 merged commit 727a4bd into main Nov 2, 2021

ashharrison90 deleted the ash/navbar-enhancements branch November 2, 2021 11:19

AgnesToulet mentioned this pull request Nov 3, 2021

Navigation: remove redundant config nav link #41252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigation: New NavBar designs behind feature toggle #41045

Navigation: New NavBar designs behind feature toggle #41045

ashharrison90 commented Oct 28, 2021

hugohaggmark Oct 29, 2021

hugohaggmark left a comment

hugohaggmark Oct 29, 2021

hugohaggmark Oct 29, 2021

hugohaggmark Oct 29, 2021

ashharrison90 commented Oct 29, 2021

hugohaggmark commented Oct 29, 2021

torkelo commented Oct 29, 2021

ashharrison90 commented Oct 29, 2021

torkelo commented Oct 29, 2021

lukasztyrala commented Oct 29, 2021

hugohaggmark left a comment

hugohaggmark Nov 1, 2021

ashharrison90 Nov 1, 2021

hugohaggmark Nov 1, 2021

ashharrison90 Nov 1, 2021

hugohaggmark Nov 1, 2021

ashharrison90 Nov 1, 2021

hugohaggmark Nov 1, 2021

mckn Nov 1, 2021

mckn left a comment

ashharrison90 commented Nov 1, 2021 •

edited

Loading

Navigation: New NavBar designs behind feature toggle #41045

Navigation: New NavBar designs behind feature toggle #41045

Conversation

ashharrison90 commented Oct 28, 2021

Choose a reason for hiding this comment

hugohaggmark left a 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

ashharrison90 commented Oct 29, 2021

hugohaggmark commented Oct 29, 2021

torkelo commented Oct 29, 2021

ashharrison90 commented Oct 29, 2021

torkelo commented Oct 29, 2021

lukasztyrala commented Oct 29, 2021

hugohaggmark left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckn left a comment

Choose a reason for hiding this comment

ashharrison90 commented Nov 1, 2021 • edited Loading

ashharrison90 commented Nov 1, 2021 •

edited

Loading