From 08200bc533e14552e3236c9e9ba8d5359387652b Mon Sep 17 00:00:00 2001 From: Tom Ratcliffe Date: Tue, 23 Apr 2024 11:04:53 +0100 Subject: [PATCH] Nav: Fix alerting links/special cases not selecting the right MegaMenu item (#85336) * Render current MegaMenu link as `aria-current=page` * Add overrides capability for mega menu links * Pass pageNav into getActiveItem so we can use override capability * Test MegaMenu special cases for starred & dashboards * Test that overrides for megamenu util works correctly * Alpha-sort megamenu overrides * Refactor util for getting active item for megamenu Update parameters to getActiveItem Update tests for getActiveItem * Fix test for starred dashboard and remove query param test Query param case happens differently in real app and is fiddly to test here * handle edge cases * restore handling home page test * fix dashboard settings * handle starring properly --------- Co-authored-by: Ashley Harrison --- .../AppChrome/MegaMenu/MegaMenu.test.tsx | 3 +- .../AppChrome/MegaMenu/MegaMenu.tsx | 2 +- .../AppChrome/MegaMenu/MegaMenuItemText.tsx | 1 + .../AppChrome/MegaMenu/utils.test.ts | 142 +++++------------- .../components/AppChrome/MegaMenu/utils.ts | 86 +++++------ public/app/core/reducers/navModel.ts | 4 + .../dashboard/components/DashNav/DashNav.tsx | 32 +++- .../DashboardSettings/DashboardSettings.tsx | 7 +- .../dashboard/containers/DashboardPage.tsx | 7 +- .../app/plugins/panel/dashlist/DashList.tsx | 24 ++- 10 files changed, 145 insertions(+), 163 deletions(-) diff --git a/public/app/core/components/AppChrome/MegaMenu/MegaMenu.test.tsx b/public/app/core/components/AppChrome/MegaMenu/MegaMenu.test.tsx index 43f9bc20180..68c4519423e 100644 --- a/public/app/core/components/AppChrome/MegaMenu/MegaMenu.test.tsx +++ b/public/app/core/components/AppChrome/MegaMenu/MegaMenu.test.tsx @@ -2,14 +2,13 @@ import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; import { Router } from 'react-router-dom'; +import { TestProvider } from 'test/helpers/TestProvider'; import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock'; import { NavModelItem } from '@grafana/data'; import { selectors } from '@grafana/e2e-selectors'; import { locationService } from '@grafana/runtime'; -import { TestProvider } from '../../../../../test/helpers/TestProvider'; - import { MegaMenu } from './MegaMenu'; const setup = () => { diff --git a/public/app/core/components/AppChrome/MegaMenu/MegaMenu.tsx b/public/app/core/components/AppChrome/MegaMenu/MegaMenu.tsx index 4f55f840990..7cb0e65dd83 100644 --- a/public/app/core/components/AppChrome/MegaMenu/MegaMenu.tsx +++ b/public/app/core/components/AppChrome/MegaMenu/MegaMenu.tsx @@ -32,7 +32,7 @@ export const MegaMenu = React.memo( .filter((item) => item.id !== 'profile' && item.id !== 'help') .map((item) => enrichWithInteractionTracking(item, state.megaMenuDocked)); - const activeItem = getActiveItem(navItems, location.pathname); + const activeItem = getActiveItem(navItems, state.sectionNav.node, location.pathname); const handleDockedMenu = () => { chrome.setMegaMenuDocked(!state.megaMenuDocked); diff --git a/public/app/core/components/AppChrome/MegaMenu/MegaMenuItemText.tsx b/public/app/core/components/AppChrome/MegaMenu/MegaMenuItemText.tsx index 6510fda9a27..f05cbd54ee9 100644 --- a/public/app/core/components/AppChrome/MegaMenu/MegaMenuItemText.tsx +++ b/public/app/core/components/AppChrome/MegaMenu/MegaMenuItemText.tsx @@ -38,6 +38,7 @@ export function MegaMenuItemText({ children, isActive, onClick, target, url }: P href={url} target={target} onClick={onClick} + {...(isActive && { 'aria-current': 'page' })} > {linkContent} diff --git a/public/app/core/components/AppChrome/MegaMenu/utils.test.ts b/public/app/core/components/AppChrome/MegaMenu/utils.test.ts index eacea3cfdf9..e03c46022e7 100644 --- a/public/app/core/components/AppChrome/MegaMenu/utils.test.ts +++ b/public/app/core/components/AppChrome/MegaMenu/utils.test.ts @@ -1,7 +1,7 @@ -import { GrafanaConfig, locationUtil, NavModelItem } from '@grafana/data'; +import { NavModelItem } from '@grafana/data'; import { ContextSrv, setContextSrv } from 'app/core/services/context_srv'; -import { enrichHelpItem, getActiveItem, isMatchOrChildMatch } from './utils'; +import { enrichHelpItem, getActiveItem } from './utils'; jest.mock('../../../app_events', () => ({ publish: jest.fn(), @@ -44,143 +44,83 @@ describe('enrichConfigItems', () => { }); }); -describe('isMatchOrChildMatch', () => { - const mockChild: NavModelItem = { - text: 'Child', - url: '/dashboards/child', - }; - const mockItemToCheck: NavModelItem = { - text: 'Dashboards', - url: '/dashboards', - children: [mockChild], - }; - - it('returns true if the itemToCheck is an exact match with the searchItem', () => { - const searchItem = mockItemToCheck; - expect(isMatchOrChildMatch(mockItemToCheck, searchItem)).toBe(true); - }); - - it('returns true if the itemToCheck has a child that matches the searchItem', () => { - const searchItem = mockChild; - expect(isMatchOrChildMatch(mockItemToCheck, searchItem)).toBe(true); - }); - - it('returns false otherwise', () => { - const searchItem: NavModelItem = { - text: 'No match', - url: '/noMatch', - }; - expect(isMatchOrChildMatch(mockItemToCheck, searchItem)).toBe(false); - }); -}); - describe('getActiveItem', () => { + const starredDashboardUid = 'foo'; const mockNavTree: NavModelItem[] = [ { text: 'Item', url: '/item', - }, - { - text: 'Item with query param', - url: '/itemWithQueryParam?foo=bar', - }, - { - text: 'Item after subpath', - url: '/subUrl/itemAfterSubpath', + id: 'item', }, { text: 'Item with children', url: '/itemWithChildren', + id: 'item-with-children', children: [ { text: 'Child', url: '/child', + id: 'child', }, ], }, - { - text: 'Alerting item', - url: '/alerting/list', - }, { text: 'Base', url: '/', + id: 'home', }, { text: 'Starred', url: '/dashboards?starred', id: 'starred', + children: [ + { + id: `starred/${starredDashboardUid}`, + text: 'Lazy Loading', + url: `/d/${starredDashboardUid}/some-name`, + }, + ], }, { text: 'Dashboards', url: '/dashboards', - }, - { - text: 'More specific dashboard', - url: '/d/moreSpecificDashboard', + id: 'dashboards', }, ]; - beforeEach(() => { - locationUtil.initialize({ - config: { appSubUrl: '/subUrl' } as GrafanaConfig, - getVariablesUrlParams: () => ({}), - getTimeRangeForUrl: () => ({ from: 'now-7d', to: 'now' }), - }); - }); it('returns an exact match at the top level', () => { - const mockPathName = '/item'; - expect(getActiveItem(mockNavTree, mockPathName)).toEqual({ - text: 'Item', - url: '/item', - }); + const mockPage: NavModelItem = { + text: 'Some current page', + id: 'item', + }; + expect(getActiveItem(mockNavTree, mockPage)?.id).toEqual('item'); }); - it('returns an exact match ignoring root subpath', () => { - const mockPathName = '/itemAfterSubpath'; - expect(getActiveItem(mockNavTree, mockPathName)).toEqual({ - text: 'Item after subpath', - url: '/subUrl/itemAfterSubpath', - }); - }); - - it('returns an exact match ignoring query params', () => { - const mockPathName = '/itemWithQueryParam?bar=baz'; - expect(getActiveItem(mockNavTree, mockPathName)).toEqual({ - text: 'Item with query param', - url: '/itemWithQueryParam?foo=bar', - }); + it('returns parent item if no other matches in nav tree', () => { + const mockPage: NavModelItem = { + text: 'Some child page', + id: 'something-that-doesnt-exist', + parentItem: { + text: 'Some home page', + id: 'home', + }, + }; + expect(getActiveItem(mockNavTree, mockPage)?.id).toEqual('home'); }); it('returns an exact child match', () => { - const mockPathName = '/child'; - expect(getActiveItem(mockNavTree, mockPathName)).toEqual({ - text: 'Child', - url: '/child', - }); + const mockPage: NavModelItem = { + text: 'Some child page', + id: 'child', + }; + expect(getActiveItem(mockNavTree, mockPage)?.id).toEqual('child'); }); - it('returns the alerting link if the pathname is an alert notification', () => { - const mockPathName = '/alerting/notification/foo'; - expect(getActiveItem(mockNavTree, mockPathName)).toEqual({ - text: 'Alerting item', - url: '/alerting/list', - }); - }); - - it('returns the dashboards route link if the pathname starts with /d/', () => { - const mockPathName = '/d/foo'; - expect(getActiveItem(mockNavTree, mockPathName)).toEqual({ - text: 'Dashboards', - url: '/dashboards', - }); - }); - - it('returns a more specific link if one exists', () => { - const mockPathName = '/d/moreSpecificDashboard'; - expect(getActiveItem(mockNavTree, mockPathName)).toEqual({ - text: 'More specific dashboard', - url: '/d/moreSpecificDashboard', - }); + it('handles home page', () => { + const mockPage: NavModelItem = { + text: 'Something else', + id: 'not-home', + }; + expect(getActiveItem(mockNavTree, mockPage, '/')?.id).toEqual('home'); }); }); diff --git a/public/app/core/components/AppChrome/MegaMenu/utils.ts b/public/app/core/components/AppChrome/MegaMenu/utils.ts index 9486c3ee5ce..90950cc25cd 100644 --- a/public/app/core/components/AppChrome/MegaMenu/utils.ts +++ b/public/app/core/components/AppChrome/MegaMenu/utils.ts @@ -1,6 +1,7 @@ -import { locationUtil, NavModelItem } from '@grafana/data'; +import { NavModelItem } from '@grafana/data'; import { config, reportInteraction } from '@grafana/runtime'; import { t } from 'app/core/internationalization'; +import { HOME_NAV_ID } from 'app/core/reducers/navModel'; import { ShowModalReactEvent } from '../../../../types/events'; import appEvents from '../../../app_events'; @@ -46,10 +47,6 @@ export const enrichWithInteractionTracking = (item: NavModelItem, megaMenuDocked return newItem; }; -export const isMatchOrChildMatch = (itemToCheck: NavModelItem, searchItem?: NavModelItem) => { - return Boolean(itemToCheck === searchItem || hasChildMatch(itemToCheck, searchItem)); -}; - export const hasChildMatch = (itemToCheck: NavModelItem, searchItem?: NavModelItem): boolean => { return Boolean( itemToCheck.children?.some((child) => { @@ -62,57 +59,48 @@ export const hasChildMatch = (itemToCheck: NavModelItem, searchItem?: NavModelIt ); }; -const stripQueryParams = (url?: string) => { - return url?.split('?')[0] ?? ''; -}; - -const isBetterMatch = (newMatch: NavModelItem, currentMatch?: NavModelItem) => { - const currentMatchUrl = stripQueryParams(currentMatch?.url); - const newMatchUrl = stripQueryParams(newMatch.url); - return newMatchUrl && newMatchUrl.length > currentMatchUrl?.length; -}; - export const getActiveItem = ( navTree: NavModelItem[], - pathname: string, - currentBestMatch?: NavModelItem + currentPage: NavModelItem, + url?: string ): NavModelItem | undefined => { - const dashboardLinkMatch = '/dashboards'; + const { id, parentItem } = currentPage; - for (const link of navTree) { - const linkWithoutParams = stripQueryParams(link.url); - const linkPathname = locationUtil.stripBaseFromUrl(linkWithoutParams); - if (linkPathname && link.id !== 'starred') { - if (linkPathname === pathname) { - // exact match - currentBestMatch = link; - break; - } else if (linkPathname !== '/' && pathname.startsWith(linkPathname)) { - // partial match - if (isBetterMatch(link, currentBestMatch)) { - currentBestMatch = link; - } - } else if (linkPathname === '/alerting/list' && pathname.startsWith('/alerting/notification/')) { - // alert channel match - // TODO refactor routes such that we don't need this custom logic - currentBestMatch = link; - break; - } else if (linkPathname === dashboardLinkMatch && pathname.startsWith('/d/')) { - // dashboard match - // TODO refactor routes such that we don't need this custom logic - if (isBetterMatch(link, currentBestMatch)) { - currentBestMatch = link; - } + // special case for the home page + if (url === '/') { + return navTree.find((item) => item.id === HOME_NAV_ID); + } + + // special case for profile as it's not part of the mega menu + if (currentPage.id === 'profile') { + return undefined; + } + + for (const navItem of navTree) { + const isIdMatch = Boolean(navItem.id && navItem.id === id); + const isTextUrlMatch = navItem.text === currentPage.text && navItem.url === currentPage.url; + + // ideally, we should only match on id + // unfortunately it's not a required property of the interface, and there are some cases + // where it's not set, particularly with child pages of plugins + // in those cases, we fall back to a text + url match + if (isIdMatch || isTextUrlMatch) { + return navItem; + } + + if (navItem.children) { + const childrenMatch = getActiveItem(navItem.children, currentPage); + if (childrenMatch) { + return childrenMatch; } } - if (link.children) { - currentBestMatch = getActiveItem(link.children, pathname, currentBestMatch); - } - if (stripQueryParams(currentBestMatch?.url) === pathname) { - return currentBestMatch; - } } - return currentBestMatch; + + if (parentItem) { + return getActiveItem(navTree, parentItem); + } + + return undefined; }; export function getEditionAndUpdateLinks(): NavModelItem[] { diff --git a/public/app/core/reducers/navModel.ts b/public/app/core/reducers/navModel.ts index d4968f54684..6717c66b350 100644 --- a/public/app/core/reducers/navModel.ts +++ b/public/app/core/reducers/navModel.ts @@ -73,6 +73,8 @@ export const updateNavIndex = createAction('navIndex/updateNavInde // Since the configuration subtitle includes the organization name, we include this action to update the org name if it changes. export const updateConfigurationSubtitle = createAction('navIndex/updateConfigurationSubtitle'); +export const removeNavIndex = createAction('navIndex/removeNavIndex'); + export const getItemWithNewSubTitle = (item: NavModelItem, subTitle: string): NavModelItem => ({ ...item, parentItem: { @@ -122,6 +124,8 @@ export const navIndexReducer = (state: NavIndex = initialState, action: AnyActio 'org-settings': getItemWithNewSubTitle(state['org-settings'], subTitle), apikeys: getItemWithNewSubTitle(state.apikeys, subTitle), }; + } else if (removeNavIndex.match(action)) { + delete state[action.payload]; } return state; diff --git a/public/app/features/dashboard/components/DashNav/DashNav.tsx b/public/app/features/dashboard/components/DashNav/DashNav.tsx index 181677d5f2b..73375c196af 100644 --- a/public/app/features/dashboard/components/DashNav/DashNav.tsx +++ b/public/app/features/dashboard/components/DashNav/DashNav.tsx @@ -15,6 +15,7 @@ import { ConfirmModal, Badge, } from '@grafana/ui'; +import { updateNavIndex } from 'app/core/actions'; import { AppChromeUpdate } from 'app/core/components/AppChrome/AppChromeUpdate'; import { NavToolbarSeparator } from 'app/core/components/AppChrome/NavToolbar/NavToolbarSeparator'; import config from 'app/core/config'; @@ -22,7 +23,8 @@ import { useAppNotification } from 'app/core/copy/appNotification'; import { appEvents } from 'app/core/core'; import { useBusEvent } from 'app/core/hooks/useBusEvent'; import { t, Trans } from 'app/core/internationalization'; -import { setStarred } from 'app/core/reducers/navBarTree'; +import { ID_PREFIX, setStarred } from 'app/core/reducers/navBarTree'; +import { removeNavIndex } from 'app/core/reducers/navModel'; import AddPanelButton from 'app/features/dashboard/components/AddPanelButton/AddPanelButton'; import { SaveDashboardDrawer } from 'app/features/dashboard/components/SaveDashboard/SaveDashboardDrawer'; import { getDashboardSrv } from 'app/features/dashboard/services/DashboardSrv'; @@ -30,7 +32,7 @@ import { DashboardModel } from 'app/features/dashboard/state'; import { DashboardInteractions } from 'app/features/dashboard-scene/utils/interactions'; import { playlistSrv } from 'app/features/playlist/PlaylistSrv'; import { updateTimeZoneForSession } from 'app/features/profile/state/reducers'; -import { KioskMode } from 'app/types'; +import { KioskMode, StoreState } from 'app/types'; import { DashboardMetaChangedEvent, ShowModalReactEvent } from 'app/types/events'; import { @@ -44,11 +46,17 @@ import { DashNavTimeControls } from './DashNavTimeControls'; import { ShareButton } from './ShareButton'; const mapDispatchToProps = { + removeNavIndex, setStarred, updateTimeZoneForSession, + updateNavIndex, }; -const connector = connect(null, mapDispatchToProps); +const mapStateToProps = (state: StoreState) => ({ + navIndex: state.navIndex, +}); + +const connector = connect(mapStateToProps, mapDispatchToProps); const selectors = e2eSelectors.pages.Dashboard.DashNav; @@ -121,10 +129,26 @@ export const DashNav = React.memo((props) => { const onStarDashboard = () => { DashboardInteractions.toolbarFavoritesClick(); const dashboardSrv = getDashboardSrv(); - const { dashboard, setStarred } = props; + const { dashboard, navIndex, removeNavIndex, setStarred, updateNavIndex } = props; dashboardSrv.starDashboard(dashboard.uid, Boolean(dashboard.meta.isStarred)).then((newState) => { setStarred({ id: dashboard.uid, title: dashboard.title, url: dashboard.meta.url ?? '', isStarred: newState }); + const starredNavItem = navIndex['starred']; + if (newState) { + starredNavItem.children?.push({ + id: ID_PREFIX + dashboard.uid, + text: dashboard.title, + url: dashboard.meta.url ?? '', + parentItem: starredNavItem, + }); + } else { + removeNavIndex(ID_PREFIX + dashboard.uid); + const indexToRemove = starredNavItem.children?.findIndex((element) => element.id === ID_PREFIX + dashboard.uid); + if (indexToRemove) { + starredNavItem.children?.splice(indexToRemove, 1); + } + } + updateNavIndex(starredNavItem); dashboard.meta.isStarred = newState; forceUpdate(); }); diff --git a/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.tsx b/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.tsx index 23f24987ca3..766dc4a6613 100644 --- a/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.tsx +++ b/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.tsx @@ -53,7 +53,7 @@ export function DashboardSettings({ dashboard, editview, pageNav, sectionNav }: const canSave = dashboard.meta.canSave; const location = useLocation(); const editIndex = getEditIndex(location); - const subSectionNav = getSectionNav(pageNav, sectionNav, pages, currentPage, location); + const subSectionNav = getSectionNav(pageNav, sectionNav, pages, currentPage, location, dashboard.uid); const size = 'sm'; const actions = [ @@ -178,7 +178,8 @@ function getSectionNav( sectionNav: NavModel, pages: SettingsPage[], currentPage: SettingsPage, - location: H.Location + location: H.Location, + dashboardUid: string ): NavModel { const main: NavModelItem = { text: t('dashboard-settings.settings.title', 'Settings'), @@ -191,7 +192,7 @@ function getSectionNav( main.children = pages.map((page) => ({ text: page.title, icon: page.icon, - id: page.id, + id: `${dashboardUid}/${page.id}`, url: locationUtil.getUrlForPartial(location, { editview: page.id, editIndex: null }), active: page === currentPage, parentItem: main, diff --git a/public/app/features/dashboard/containers/DashboardPage.tsx b/public/app/features/dashboard/containers/DashboardPage.tsx index 870be183763..2f1b1e16fb8 100644 --- a/public/app/features/dashboard/containers/DashboardPage.tsx +++ b/public/app/features/dashboard/containers/DashboardPage.tsx @@ -13,6 +13,7 @@ import { GrafanaContext, GrafanaContextType } from 'app/core/context/GrafanaCont import { createErrorNotification } from 'app/core/copy/appNotification'; import { getKioskMode } from 'app/core/navigation/kiosk'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; +import { ID_PREFIX } from 'app/core/reducers/navBarTree'; import { getNavModel } from 'app/core/selectors/navModel'; import { PanelModel } from 'app/features/dashboard/state'; import { dashboardWatcher } from 'app/features/live/dashboard/dashboardWatcher'; @@ -477,7 +478,11 @@ function updateStatePageNavFromProps(props: Props, state: State): State { pageNav.parentItem = pageNav.parentItem; } } else { - sectionNav = getNavModel(props.navIndex, 'dashboards/browse'); + sectionNav = getNavModel( + props.navIndex, + ID_PREFIX + dashboard.uid, + getNavModel(props.navIndex, 'dashboards/browse') + ); } if (state.editPanel || state.viewPanel) { diff --git a/public/app/plugins/panel/dashlist/DashList.tsx b/public/app/plugins/panel/dashlist/DashList.tsx index e0a75e7e33b..876a3dd1b8c 100644 --- a/public/app/plugins/panel/dashlist/DashList.tsx +++ b/public/app/plugins/panel/dashlist/DashList.tsx @@ -11,16 +11,18 @@ import { urlUtil, } from '@grafana/data'; import { CustomScrollbar, useStyles2, IconButton } from '@grafana/ui'; +import { updateNavIndex } from 'app/core/actions'; import { getConfig } from 'app/core/config'; import { appEvents } from 'app/core/core'; import { useBusEvent } from 'app/core/hooks/useBusEvent'; -import { setStarred } from 'app/core/reducers/navBarTree'; +import { ID_PREFIX, setStarred } from 'app/core/reducers/navBarTree'; +import { removeNavIndex } from 'app/core/reducers/navModel'; import { getBackendSrv } from 'app/core/services/backend_srv'; import impressionSrv from 'app/core/services/impression_srv'; import { getDashboardSrv } from 'app/features/dashboard/services/DashboardSrv'; import { DashboardSearchItem } from 'app/features/search/types'; import { VariablesChanged } from 'app/features/variables/types'; -import { useDispatch } from 'app/types'; +import { useDispatch, useSelector } from 'app/types'; import { Options } from './panelcfg.gen'; import { getStyles } from './styles'; @@ -102,6 +104,7 @@ async function fetchDashboards(options: Options, replaceVars: InterpolateFunctio export function DashList(props: PanelProps) { const [dashboards, setDashboards] = useState(new Map()); const dispatch = useDispatch(); + const navIndex = useSelector((state) => state.navIndex); useEffect(() => { fetchDashboards(props.options, props.replaceVariables).then((dashes) => { @@ -119,6 +122,23 @@ export function DashList(props: PanelProps) { updatedDashboards.set(dash?.uid ?? '', { ...dash, isStarred }); setDashboards(updatedDashboards); dispatch(setStarred({ id: uid ?? '', title, url, isStarred })); + + const starredNavItem = navIndex['starred']; + if (isStarred) { + starredNavItem.children?.push({ + id: ID_PREFIX + uid, + text: title, + url: url ?? '', + parentItem: starredNavItem, + }); + } else { + dispatch(removeNavIndex(ID_PREFIX + uid)); + const indexToRemove = starredNavItem.children?.findIndex((element) => element.id === ID_PREFIX + uid); + if (indexToRemove) { + starredNavItem.children?.splice(indexToRemove, 1); + } + } + dispatch(updateNavIndex(starredNavItem)); }; const [starredDashboards, recentDashboards, searchedDashboards] = useMemo(() => {