From 39f787cbefde547483d8e7626efea5a3460ef627 Mon Sep 17 00:00:00 2001
From: Ashley Harrison <ashley.harrison@grafana.com>
Date: Tue, 21 Dec 2021 16:15:03 +0000
Subject: [PATCH] Navigation: Remove some redundant css and tidy up reverse
 logic (#43403)

* Remove some redundant css and tidy up reverse logic

* Fix alignment of subtitle

* Remove navbar-dropdown class
---
 .../app/core/components/NavBar/NavBarItem.tsx | 17 +-----
 .../core/components/NavBar/NavBarItemMenu.tsx | 54 ++++++-------------
 .../components/NavBar/NavBarItemMenuItem.tsx  |  2 +-
 .../NavBar/NavBarItemMenuTrigger.tsx          | 16 ------
 4 files changed, 18 insertions(+), 71 deletions(-)

diff --git a/public/app/core/components/NavBar/NavBarItem.tsx b/public/app/core/components/NavBar/NavBarItem.tsx
index 974d1d4a04a..84648fb38d2 100644
--- a/public/app/core/components/NavBar/NavBarItem.tsx
+++ b/public/app/core/components/NavBar/NavBarItem.tsx
@@ -35,7 +35,7 @@ const NavBarItem = ({
     .filter((item) => !item.hideFromMenu)
     .map((i) => ({ ...i, menuItemType: NavMenuItemType.Item }));
   const adjustHeightForBorder = filteredItems.length === 0;
-  const styles = getStyles(theme, adjustHeightForBorder, isActive, reverseMenuDirection);
+  const styles = getStyles(theme, adjustHeightForBorder, isActive);
   const section: NavModelItem = {
     ...link,
     children: filteredItems,
@@ -115,12 +115,7 @@ const NavBarItem = ({
 
 export default NavBarItem;
 
-const getStyles = (
-  theme: GrafanaTheme2,
-  adjustHeightForBorder: boolean,
-  isActive?: boolean,
-  reverseMenuDirection?: boolean
-) => ({
+const getStyles = (theme: GrafanaTheme2, adjustHeightForBorder: boolean, isActive?: boolean) => ({
   ...getNavBarItemWithoutMenuStyles(theme, isActive),
   header: css`
     color: ${theme.colors.text.primary};
@@ -134,12 +129,4 @@ const getStyles = (
   item: css`
     color: ${theme.colors.text.primary};
   `,
-  subtitle: css`
-      border-${reverseMenuDirection ? 'bottom' : 'top'}: 1px solid ${theme.colors.border.weak};
-      color: ${theme.colors.text.secondary};
-      font-size: ${theme.typography.bodySmall.fontSize};
-      font-weight: ${theme.typography.bodySmall.fontWeight};
-      padding: ${theme.spacing(1)} ${theme.spacing(2)} ${theme.spacing(1)};
-      white-space: nowrap;
-    `,
 });
diff --git a/public/app/core/components/NavBar/NavBarItemMenu.tsx b/public/app/core/components/NavBar/NavBarItemMenu.tsx
index 7b3e9659247..000b996dc0e 100644
--- a/public/app/core/components/NavBar/NavBarItemMenu.tsx
+++ b/public/app/core/components/NavBar/NavBarItemMenu.tsx
@@ -25,7 +25,7 @@ export function NavBarItemMenu(props: NavBarItemMenuProps): ReactElement | null
   };
   const { menuHasFocus, menuProps: contextMenuProps = {} } = contextProps;
   const theme = useTheme2();
-  const styles = getStyles(theme, adjustHeightForBorder, reverseMenuDirection);
+  const styles = getStyles(theme, reverseMenuDirection);
   const state = useTreeState<NavModelItem>({ ...rest, disabledKeys });
   const ref = useRef(null);
   const { menuProps } = useMenu(completeProps, { ...state }, ref);
@@ -54,38 +54,26 @@ export function NavBarItemMenu(props: NavBarItemMenuProps): ReactElement | null
     <NavBarItemMenuItem key={section.key} item={section} state={state} onNavigate={onNavigate} />
   );
 
-  const subTitleComponent = (
-    <li key={menuSubTitle} className={styles.menuItem}>
-      <div className={styles.subtitle}>{menuSubTitle}</div>
+  const itemComponents = items.map((item) => (
+    <NavBarItemMenuItem key={getNavModelItemKey(item.value)} item={item} state={state} onNavigate={onNavigate} />
+  ));
+
+  const subTitleComponent = menuSubTitle && (
+    <li key={menuSubTitle} className={styles.subtitle}>
+      {menuSubTitle}
     </li>
   );
 
+  const menu = [sectionComponent, itemComponents, subTitleComponent];
+
   return (
-    <ul
-      className={`${styles.menu} navbar-dropdown`}
-      ref={ref}
-      {...mergeProps(menuProps, contextMenuProps)}
-      tabIndex={menuHasFocus ? 0 : -1}
-    >
-      {!reverseMenuDirection ? sectionComponent : null}
-      {menuSubTitle && reverseMenuDirection ? subTitleComponent : null}
-      {items.map((item, index) => {
-        return (
-          <NavBarItemMenuItem key={getNavModelItemKey(item.value)} item={item} state={state} onNavigate={onNavigate} />
-        );
-      })}
-      {reverseMenuDirection ? sectionComponent : null}
-      {menuSubTitle && !reverseMenuDirection ? subTitleComponent : null}
+    <ul className={styles.menu} ref={ref} {...mergeProps(menuProps, contextMenuProps)} tabIndex={menuHasFocus ? 0 : -1}>
+      {reverseMenuDirection ? menu.reverse() : menu}
     </ul>
   );
 }
 
-function getStyles(
-  theme: GrafanaTheme2,
-  adjustHeightForBorder: boolean,
-  reverseDirection?: boolean,
-  isFocused?: boolean
-) {
+function getStyles(theme: GrafanaTheme2, reverseDirection?: boolean) {
   return {
     menu: css`
       background-color: ${theme.colors.background.primary};
@@ -103,26 +91,14 @@ function getStyles(
       z-index: ${theme.zIndex.sidemenu};
       list-style: none;
     `,
-    menuItem: css`
-      background-color: ${isFocused ? theme.colors.action.hover : 'transparent'};
-      color: ${isFocused ? 'white' : theme.colors.text.primary};
-
-      &:focus-visible {
-        background-color: ${theme.colors.action.hover};
-        box-shadow: none;
-        color: ${theme.colors.text.primary};
-        outline: 2px solid ${theme.colors.primary.main};
-        // Need to add condition, header is 0, otherwise -2
-        outline-offset: -0px;
-        transition: none;
-      }
-    `,
     subtitle: css`
+      background-color: transparent;
       border-${reverseDirection ? 'bottom' : 'top'}: 1px solid ${theme.colors.border.weak};
       color: ${theme.colors.text.secondary};
       font-size: ${theme.typography.bodySmall.fontSize};
       font-weight: ${theme.typography.bodySmall.fontWeight};
       padding: ${theme.spacing(1)} ${theme.spacing(2)} ${theme.spacing(1)};
+      text-align: left;
       white-space: nowrap;
     `,
   };
diff --git a/public/app/core/components/NavBar/NavBarItemMenuItem.tsx b/public/app/core/components/NavBar/NavBarItemMenuItem.tsx
index 8384388c478..785a4f3ac0a 100644
--- a/public/app/core/components/NavBar/NavBarItemMenuItem.tsx
+++ b/public/app/core/components/NavBar/NavBarItemMenuItem.tsx
@@ -72,7 +72,7 @@ function getStyles(theme: GrafanaTheme2, isFocused: boolean, isSection: boolean)
   return {
     menuItem: css`
       background-color: ${backgroundColor};
-      color: ${isFocused ? 'white' : theme.colors.text.primary};
+      color: ${theme.colors.text.primary};
 
       &:focus-visible {
         background-color: ${theme.colors.action.hover};
diff --git a/public/app/core/components/NavBar/NavBarItemMenuTrigger.tsx b/public/app/core/components/NavBar/NavBarItemMenuTrigger.tsx
index c96e260543b..c93dcf0ff73 100644
--- a/public/app/core/components/NavBar/NavBarItemMenuTrigger.tsx
+++ b/public/app/core/components/NavBar/NavBarItemMenuTrigger.tsx
@@ -164,22 +164,6 @@ export function NavBarItemMenuTrigger(props: NavBarItemMenuTriggerProps): ReactE
 }
 
 const getStyles = (theme: GrafanaTheme2, isActive?: boolean) => ({
-  container: css`
-    position: relative;
-    color: ${isActive ? theme.colors.text.primary : theme.colors.text.secondary};
-    list-style: none;
-
-    &:hover {
-      background-color: ${theme.colors.action.hover};
-      color: ${theme.colors.text.primary};
-
-      // TODO don't use a hardcoded class here, use isVisible in NavBarDropdown
-      .navbar-dropdown {
-        opacity: 1;
-        visibility: visible;
-      }
-    }
-  `,
   element: css`
     background-color: transparent;
     border: none;