Fix sidenavs not redering for lovell facility pages #226
Conversation
src/lib/drupal/facilitySideNav.ts
Outdated
| return { | ||
| description: item.description, | ||
| expanded: item.expanded, | ||
| label: item.title, | ||
| links: nestedItems, | ||
| url: { path: item.url }, | ||
| fieldMenuSection: item.field_menu_section || null, |
There was a problem hiding this comment.
CONSIDER
I think I'd prefer this to not be field.... A big part of the formatter step, I think, is to move away from the Drupal-specific naming. Maybe menuSection or just section. Or, if this is only for Lovell and likely only ever will be, then maybe lovellSection?
There was a problem hiding this comment.
completely agree here. lovellSection feels appropriate, and clearer for future debuggers
There was a problem hiding this comment.
Makes sense to me, fixed
src/lib/drupal/lovell/utils.ts
Outdated
| @@ -133,3 +133,34 @@ export function isLovellBifurcatedResource( | |||
| isLovellFederalResource(resource as LovellBifurcatedFormattedResource) | |||
| ) | |||
| } | |||
|
|
|||
| export function getLovellVariantOfMenu(menu, variant: LovellVariant) { | |||
There was a problem hiding this comment.
SHOULD
I think we should likely define a type for menu.
Also, noting here because it's semi-related, but not related to code from this PR (I noticed it when examining what a type definition for menu might look like): buildSideNavDataFromMenu (and, more specifically, the function it calls, normalizeMenuData) is not handling Lovell correctly. This is the data that comes from that call:
{
rootPath: "/lovell-federal-health-care-tricare/stories/",
data: {
name: "Lovell Federal health care - TRICARE",
....
....
...
},
}
The name of the Lovell menu should be the Federal name, likely. It's different than all the others in that it has three top-level items. This is just grabbing the first one, which is TRICARE. It might not ultimately matter, but it is technically not correct, and might make things confusing. Or, perhaps, this should not be setting a name at all at this point. I don't want to put a ton of specific business logic in at this layer, so I'd love to see this more generally handle menu data rather than assuming that there's only one top-level item. I think we (@jtmst, @ryguyk, @tjheffner) might want to sync up on this and chat about some of the architectural implications.
There was a problem hiding this comment.
Going to leave this as is to be addressed in future issue per this discussion: https://dsva.slack.com/archives/C01SR56755H/p1698932592500079
src/lib/drupal/lovell/utils.ts
Outdated
| // Filter arrays | ||
| if (Array.isArray(menu)) { | ||
| return menu | ||
| .map((item) => getLovellVariantOfMenu(item, variant)) | ||
| .filter((item) => item !== null) | ||
| } | ||
|
|
||
| // Filter objects | ||
| const newData = {} | ||
| for (const key in menu) { | ||
| const filteredData = getLovellVariantOfMenu(menu[key], variant) | ||
| if (filteredData !== null) { | ||
| newData[key] = filteredData | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure about this. Let's sync up and discuss some things.
There was a problem hiding this comment.
I understand what's going on here, but it feels a bit weird to me. It took me a bit to wrap my head around why there's "Filter arrays" and "Filter objects". I think that kind of muddies the situation a bit. It doesn't seem like we'd ever need to worry about objects separately, as the recursion should always be on the links property.
The following code might not be perfect, as it depends on some of the other restructuring that we discussed, but it seems to me that this function should look something like this (general idea):
return menu
.filter(
(menuItem) =>
menuItem.fieldMenuSection === variant ||
menuItem.fieldMenuSection === 'both'
)
.map((menuItem) => ({
...menuItem,
links: getLovellVariantOfMenu(menuItem.links, variant),
}))
There was a problem hiding this comment.
Updated this logic for clarity, thanks for the feedback
Description
Relates to #14745. (or closes?)
Testing done
Local visual testing
Screenshots
QA steps
Navigate to lovell pages (/lovell-federal-health-care-tricare/stories/)
Verify that sidenav is rendering
Verify that links are correct with no duplicates
Verify that non-lovell page sidenavs are unchanged (/butler-health-care/stories/)
Is this PR blocked by another PR?
DO NOT MERGElabel