Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sidenavs not redering for lovell facility pages #226

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Fix sidenavs not redering for lovell facility pages #226

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7495404
tests
jtmst Oct 31, 2023
57d7121
Cleanup / PR feedback
jtmst Nov 2, 2023
c6bb833
field_section name adjust
jtmst Nov 2, 2023
aa68c16
Merge branch 'main' into 14745-lovell-sidenavs
jtmst Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/lib/drupal/facilitySideNav.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,25 @@ const normalizeMenuItem = (item: MenuItem): SideNavItem => {
if (item.items && item.items.length > 0) {
item.items.forEach((i) => nestedItems.push(normalizeMenuItem(i)))
}

return {
description: item.description,
expanded: item.expanded,
label: item.title,
links: nestedItems,
url: { path: item.url },
fieldMenuSection: item.field_menu_section || null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely agree here. lovellSection feels appropriate, and clearer for future debuggers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, fixed

}
}

const normalizeMenuData = (menu: Menu): SideNavData => {
// Bail early if no tree is provided
if (!menu.tree || menu.tree.length === 0) return null

const links = []
let links = []
menu.tree.forEach((item) => {
return links.push(normalizeMenuItem(item))
})

links = links.filter((link) => link.links.length > 0)
tjheffner marked this conversation as resolved.
Show resolved Hide resolved
return {
name: menu.tree[0].title,
description: menu.tree[0].description,
Expand All @@ -43,7 +43,6 @@ export const buildSideNavDataFromMenu = (
menu: Menu
): SideNavMenu => {
const data = normalizeMenuData(menu)

return {
rootPath: `${entityPath}/`,
data,
Expand Down
8 changes: 7 additions & 1 deletion src/lib/drupal/lovell/staticProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
getLovellVariantOfUrl,
getOppositeChildVariant,
isLovellBifurcatedResource,
getLovellVariantOfMenu,
} from './utils'

export function getLovellStaticPropsContext(
Expand Down Expand Up @@ -153,11 +154,16 @@ export async function getLovellStaticPropsResource(
): Promise<LovellStaticPropsResource<LovellFormattedResource>> {
// Lovell listing pages need Federal items merged
if (context.lovell.isLovellVariantPage && context.listing.isListingPage) {
return getLovellListingPageStaticPropsResource(
const resource = await getLovellListingPageStaticPropsResource(
resourceType as ListingResourceTypeType,
pathInfo,
context
)

return {
...resource,
menu: getLovellVariantOfMenu(resource.menu, context.lovell.variant),
}
}

// Other Lovell pages depend on base resource
Expand Down
124 changes: 124 additions & 0 deletions src/lib/drupal/lovell/tests/mockData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,127 @@ export const newsStoryPartialResource = {
listing: 'listing',
entityId: 12345,
}

export const lovellSidenavData = {
rootPath: '/lovell-federal-health-care-va/stories/',
data: {
name: 'Lovell Federal health care - TRICARE',
description: null,
links: [
{
description: null,
expanded: false,
label: 'Lovell Federal health care',
links: [
{
description: null,
expanded: false,
label: 'SERVICES AND LOCATIONS',
links: [
{
description: null,
expanded: false,
label: 'Health services',
links: [
{
description: null,
expanded: false,
label: 'Returning service member care',
links: [],
url: {
path: '/lovell-federal-health-care-tricare/health-services/returning-service-member-care',
},
fieldMenuSection: 'tricare',
},
],
url: {
path: '/lovell-federal-health-care-va/health-services',
},
fieldMenuSection: 'va',
},
{
description: null,
expanded: false,
label: 'Health services',
links: [],
url: {
path: '/lovell-federal-health-care-tricare/health-services',
},
fieldMenuSection: 'tricare',
},
{
description: null,
expanded: false,
label: 'Locations',
links: [
{
description: null,
expanded: false,
label: 'Captain James A. Lovell Federal Health Care Center',
links: [],
url: {
path: '/lovell-federal-health-care-va/locations/captain-james-a-lovell-federal-health-care-center',
},
fieldMenuSection: 'va',
},
],
url: { path: '/lovell-federal-health-care-va/locations' },
fieldMenuSection: 'va',
},
{
description: 'TRICARE locations',
expanded: false,
label: 'Locations',
links: [
{
description: null,
expanded: false,
label: 'Captain James A. Lovell Federal Health Care Center',
links: [],
url: {
path: '/lovell-federal-health-care-tricare/locations/captain-james-a-lovell-federal-health-care-center',
},
fieldMenuSection: 'tricare',
},
],
url: {
path: '/lovell-federal-health-care-tricare/locations',
},
fieldMenuSection: 'tricare',
},
],
url: { path: '' },
fieldMenuSection: 'both',
},
{
description: null,
expanded: false,
label: 'NEWS AND EVENTS',
links: [
{
description: null,
expanded: false,
label: 'Events',
links: [],
url: { path: '/lovell-federal-health-care-va/events' },
fieldMenuSection: 'va',
},
{
description: null,
expanded: false,
label: 'Events',
links: [],
url: { path: '/lovell-federal-health-care-tricare/events' },
fieldMenuSection: 'tricare',
},
],
url: { path: '' },
fieldMenuSection: 'both',
},
],
url: { path: '/lovell-federal-health-care' },
fieldMenuSection: 'both',
},
],
},
}
58 changes: 58 additions & 0 deletions src/lib/drupal/lovell/tests/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
isLovellChildVariantSlug,
getOppositeChildVariant,
isLovellBifurcatedResource,
getLovellVariantOfMenu,
} from '../utils'
import {
lovellTricareSlug,
Expand All @@ -26,6 +27,7 @@ import {
lovellVaResource,
otherResource,
newsStoryPartialResource,
lovellSidenavData,
} from './mockData'

describe('isLovellResourceType', () => {
Expand Down Expand Up @@ -319,3 +321,59 @@ describe('isLovellBifurcatedResource', () => {
expect(result).toBe(false)
})
})

describe('getLovellVariantOfMenu', () => {
it('should filter out menus with non-matching fieldMenuSection', () => {
const result = getLovellVariantOfMenu(lovellSidenavData, 'va')

// Check if the TRICARE specific items are filtered out
expect(
result.data.links[0].links[1].links.find(
(link) => link.fieldMenuSection === 'tricare'
)
).toBeUndefined()
})

it('should not filter out menus with fieldMenuSection set to "both"', () => {
const result = getLovellVariantOfMenu(lovellSidenavData, 'va')

// Check if items with fieldMenuSection set to "both" are still there
expect(result.data.links[0].fieldMenuSection).toEqual('both')
expect(result.data.links[0].links[0].fieldMenuSection).toEqual('both')
})

it('should keep VA specific items when variant is "va"', () => {
const result = getLovellVariantOfMenu(lovellSidenavData, 'va')

// Check if VA specific items are still there
expect(result.data.links[0].links[0].links[0].fieldMenuSection).toEqual(
'va'
)

// Assert against the 'Locations' sub-submenu item
expect(result.data.links[0].links[0].links[1].label).toEqual('Locations')
expect(result.data.links[0].links[0].links[1].fieldMenuSection).toEqual(
'va'
)
})

it('should filter out VA specific items when variant is "tricare"', () => {
const result = getLovellVariantOfMenu(lovellSidenavData, 'tricare')

// Check if VA specific items are filtered out
expect(
result.data.links[0].links[0].links.find(
(link) => link.fieldMenuSection === 'va'
)
).toBeUndefined()
})

it('should keep TRICARE specific items when variant is "tricare"', () => {
const result = getLovellVariantOfMenu(lovellSidenavData, 'tricare')

// Check if TRICARE specific items are still there
expect(result.data.links[0].links[0].links[1].fieldMenuSection).toEqual(
'tricare'
)
})
})
31 changes: 31 additions & 0 deletions src/lib/drupal/lovell/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,34 @@ export function isLovellBifurcatedResource(
isLovellFederalResource(resource as LovellBifurcatedFormattedResource)
)
}

export function getLovellVariantOfMenu(menu, variant: LovellVariant) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to leave this as is to be addressed in future issue per this discussion: https://dsva.slack.com/archives/C01SR56755H/p1698932592500079

tjheffner marked this conversation as resolved.
Show resolved Hide resolved
if (!menu || typeof menu !== 'object') return menu

// Check if the object has the fieldMenuSection and it doesn't match the variant or is 'both'
if (
menu.fieldMenuSection &&
menu.fieldMenuSection !== variant &&
menu.fieldMenuSection !== 'both'
) {
return null
}

// 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
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. Let's sync up and discuss some things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
    }))

Copy link
Contributor Author

@jtmst jtmst Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this logic for clarity, thanks for the feedback


return newData
}
1 change: 1 addition & 0 deletions src/types/dataTypes/drupal/menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export interface MenuItem {
expanded: boolean
enabled: boolean
items?: Tree
field_menu_section?: string
}

type Tree = ReadonlyArray<MenuItem>
Expand Down
1 change: 1 addition & 0 deletions src/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ export type SideNavItem = {
label: string
links: SideNavItem[]
url: { path: string }
fieldMenuSection?: string
}

export type SideNavData = {
Expand Down
Loading