-
Notifications
You must be signed in to change notification settings - Fork 7
Locations Listing Sidenav #1085
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.
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
Conversation
src/data/queries/locationsListing.ts
Outdated
return entity | ||
if (!entity) { | ||
throw new Error( | ||
`NodeVamcSystemVaPolice entity not found for id: ${opts.id}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be NodeLocationsListing
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I was copy pasta-ing this from other places and missed updating this!
@@ -25,9 +25,28 @@ exports[`LocationsListing formatData outputs formatted data 1`] = ` | |||
}, | |||
], | |||
"entityId": 3097, | |||
"entityPath": "/lebanon-health-care/locations", | |||
"entityPath": "/boston-health-care/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be boston-health-care/locations/
right?
@@ -129,6 +148,7 @@ exports[`LocationsListing formatData outputs formatted data 1`] = ` | |||
}, | |||
], | |||
"moderationState": "published", | |||
"path": "/boston-health-care/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here boston-health-care/locations/
// Patch mock to ensure path.alias exists and menu has a valid tree | ||
const patchedMock = { | ||
...LocationsListingMock, | ||
path: { alias: '/boston-health-care/', pid: 1, langcode: 'en' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh maybe this needs to get updated to boston-health-care/locations/
to fix the snapshot thing I was commenting on.
<article className="usa-content"> | ||
<div>TODO: Lovell switch link</div> | ||
<main className="va-l-detail-page va-facility-page"> | ||
<div className="usa-grid usa-grid-full"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could do the v3 grid now...but up to you since that is out of scope for adding the Sidenav
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the sidenav is always sticky when it comes to migration, I might do that at a later time. I tried a bit but it broke so I am hoping once some other components are flushed out it might be better to wait until then.
@moanu404 this looks good to me but if you decide to change any of the grid stuff let me know and I can re-QA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Description
Adds the SideNav to the locations listing scaffolded page.
Generated description
This pull request introduces enhancements to the
LocationsListing
query and layout, focusing on integrating sidebar navigation functionality and improving data formatting. The most significant changes include adding support for a structured sidebar menu, updating the query logic to fetch and format menu data, and modifying theLocationsListing
layout to render the sidebar navigation dynamically.Enhancements to query logic:
src/data/queries/locationsListing.ts
: Modified theparams
function to includefield_office
in the query and updated thedata
loader to fetch and return menu data alongside the entity. Added error handling for missing entities and introduced theLocationsListingData
type to encapsulate both entity and menu data.Sidebar menu integration:
src/templates/layouts/locationsListing/index.tsx
: Added amenu
prop to theLocationsListing
component and implemented auseEffect
hook to expose the menu data globally viawindow.sideNav
. Updated the layout to include a<nav>
element for rendering the sidebar navigation. [1] [2]Data formatting improvements:
src/data/queries/locationsListing.ts
: Enhanced theformatter
function to process and format the sidebar menu data usingbuildSideNavDataFromMenu
, ensuring the menu structure is included in the formatted output.Testing updates:
src/data/queries/tests/__snapshots__/locationsListing.test.tsx.snap
: Updated snapshots to reflect the newmenu
andpath
properties in the formatted output. [1] [2]src/data/queries/tests/locationsListing.test.tsx
: Added tests to validate the inclusion offield_office
in query parameters and the proper structure of the formatted sidebar menu. Patched mock data to ensure compatibility with the new menu structure.Type updates:
src/types/formatted/locationsListing.ts
: Extended theLocationsListing
type to includemenu
andpath
properties, ensuring type safety for the new data structure.Ticket
Closes 21283
Developer Task
Testing Steps
QA steps
outlined in ticket
Screenshots
Is this PR blocked by another PR?
DO NOT MERGE
labelReviewer
Reviewing a PR
This section lists items that need to be checked or updated when making changes to this repository.
Standard Checks