Nav: Fix alerting links/special cases not selecting the right MegaMenu item #85336
Conversation
tomratcliffe
requested review from
a team,
gillesdemey,
konrad147 and
soniaAguilarPeiron
and removed request for
a team
| @@ -38,6 +38,7 @@ export function MegaMenuItemText({ children, isActive, onClick, target, url }: P | |||
| href={url} | |||
| target={target} | |||
| onClick={onClick} | |||
| {...(isActive && { 'aria-current': 'page' })} | |||
There was a problem hiding this comment.
This might be slightly inaccurate, as the href of the active item doesn't exactly match the current page (e.g. it's selecting a link that refers to /dashboards, but we might be on /d/someuid/name).
Does this seem right? Or just aria-current=true?
| 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', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
(I believe) When legacy alerting was removed, we also removed the need for this special case as the route doesn't exist anymore
There was a problem hiding this comment.
Yep, this was a legacy route so good riddance :)
tomratcliffe
added
the
no-changelog
tomratcliffe
requested review from
Clarity-89 and
JoaoSilvaGrafana
and removed request for
a team
There was a problem hiding this comment.
i think we can do even better now. looking at the url was always temporary and prone to failures. but in MegaMenu we have access to state.sectionNav.node. ignore the name, it's the active page. pretty sure we use it to construct the breadcrumbs
i think we can rewrite getActiveItem as something like:
export const getActiveItem = (
navTree: NavModelItem[],
node: NavModelItem,
): NavModelItem | undefined => {
if (!node || node.id === 'profile') {
return undefined;
}
for (const link of navTree) {
if (node.text === link.text && node.url === link.url) {
return link;
} else if (link.children) {
const activeChild = getActiveItem(link.children, node);
if (activeChild) {
return activeChild;
}
}
}
if (node.parentItem) {
return getActiveItem(navTree, node.parentItem);
}
return undefined;
};and call it in MegaMenu with:
const activeItem = getActiveItem(navItems, state.sectionNav.node);think that will then ✨ just work ✨ for everything except the home page which we'll probably have to special case somehow.
that should also let us delete the stripQueryParams, isBetterMatch and isMatchOrChildMatch functions 🤞
|
Perfect, thanks @ashharrison90, much cleaner! I had tried to do something similar but I didn't realise that I could get the current page reliably through that 😃 Will have a look and a refactor to fix it. I think the starred dashboards will also need handling differently 👍 |
Update parameters to getActiveItem Update tests for getActiveItem
tomratcliffe
force-pushed
the
alerting/mega-menu-fixes
branch
from
e1dbba9
to
ab7380b
Compare
| const stripQueryParams = (url?: string) => { | ||
| return url?.split('?')[0] ?? ''; | ||
| }; | ||
| /** |
There was a problem hiding this comment.
I still had to do some special-casing here. I looked at the dashboard page code a little bit to see if I could easily set the nav item to be a "starred" one, but I didn't fancy making changes in somewhere so critical in this PR - unless someone is able to point me in a direction to quickly fix that?
I was getting a dashboard page as only ever having a nav item with ID of dashboards/browse
Query param case happens differently in real app and is fiddly to test here
There was a problem hiding this comment.
ok this is real close, nice job 🙌
i see 2 problems here, but i think we can fix them both 🤞:
- dashboards being special cased
- let's modify the logic for the dashboard navModel
- search for a nav item matching the starred dashboard id
- fall back to
dashboards/browseotherwise
- let's modify the logic for the dashboard navModel
- visiting
Profile/Notification history/Change passwordshowsHomeas highlighted- let's just add a special case for profile:
if (currentPage.id === 'profile') { return undefined; }
i've raised a PR into this branch with those changes here
i think with those we're good to go 👍
tomratcliffe
requested review from
oscarkilhed and
kaydelaney
and removed request for
a team
|
/deploy-to-hg |
|
|
Error building instance: Contact #proj-ephemeral-hg-instances if it is not a compile error. Logs handling pull request comment event: running grafana-build deb: executing command: stdout= stderr=go: downloading dagger.io/dagger v0.11.0 Stdout: |
|
/deploy-to-hg |
|
|
|
I just checked through the ephemeral instance, all seems to be working except for one edge case that I don't think is necessarily worth fixing in here: |
ah i remember there being some special logic for this somewhere... lemme see if i can find it 🤔 |
|
@tomratcliffe have pushed a change for dashboard settings that should fix the remaining e2e tests - don't let anyone ever tell you they don't catch things! 😅 i'm still looking into whether there's a nice way to handle starred items. i agree it's not really a big deal, and if i can't find a nice solution by tomorrow i'll just stick an approve on this anyway 👍 |
|
@tomratcliffe have pushed a change to handle starring/unstarring correctly. it's not particularly nice as it means we have to update 2 different slices of the state, but at least it's not regressing behaviour. the long term solution would be to consolidate everything in one slice and move everything to rtk query then it can invalidate the cache and refetch when mutations like starring/unstarring occur, but that's a bigger task and firmly in the frontend-platform court 😅 |
|
Thanks for all your help on this one Ash! 🙌 |
What is this feature?
Fixes to rendering of the current active item in the MegaMenu
Why do we need this feature?
When rendering some URLs, the correct MegaMenu item wasn't being selected.
For example,
/alerting/silence/newwas highlighting theAlertingnav item, rather thanSilences. This is because not all URLs follow a predictable structure, so until we have a consistent hierarchy of routing, we can't rely on matching the start of URLs. For that specific case, the parent URL is/alerting/silences(plural), so thestartsWithdoesn't match up.It would be a fairly substantial overhaul of existing URLs to fix this and make them all structured consistently, so the cleanest solution right now seems to be providing a way of overriding the exact nav item that should be highlighted - any other suggestions are welcomed though!
Which issue(s) does this PR fix?:
Fixes #83507 but also fixes
Administration -> Organizations -> Newnot selecting the correct item (it was highlightingDefault preferences)