-
Notifications
You must be signed in to change notification settings - Fork 14
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
implement multiple accordions on customize query #2568
implement multiple accordions on customize query #2568
Conversation
@@ -326,3 +326,9 @@ export interface ValueSet { | |||
} | |||
|
|||
export type ValueSetType = keyof ValueSet; | |||
|
|||
export const valueSetTypeToClincalServiceTypeMap = { |
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.
I needed this information for some frontend sorting work, so I pulled it out / reimported it into the backend database service as well. Let me know if there's a better semantic name for this
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.
I can't think of something better off the top of my head! I think there's a decent chance many things get renamed as we work through tech debt in the coming weeks,
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 just you wait until we cleanup all of our seventeen mapping constants that all do the same thing...
@@ -326,3 +326,9 @@ export interface ValueSet { | |||
} | |||
|
|||
export type ValueSetType = keyof ValueSet; |
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.
Realize I snuck this in in the previous PR, but since were using the "labs | conditions | medications" set of strings in a bunch of places, went ahead and formalized it into a type. Again, would love to know if this is a sensible name / suggestions for alternatives if not
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.
ValueSetType looks good to me and is consistent with how it is referred to in the TCR
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 this works for me! It's a bit of a weird terminology thing because these three types actually inherit from something called a "Clinical Service Type" in the coding schemes, but there are six of those that map non-uniformly to these value set types here (e.g. 3 of the 6 go to labs, 2 of the 6 to conditions, and 1 just is medications). Since we're not capturing all 6, I think what you've done here makes sense.
@@ -108,37 +130,9 @@ const CustomizeQuery: React.FC<CustomizeQueryProps> = ({ | |||
}; | |||
|
|||
useEffect(() => { | |||
// Gate whether we actually update state after fetching so we |
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.
Since we're passing down the querySetValues that we need to display as a prop, no longer need this useEffect
…' of https:/CDCgov/phdi into rob-and-bob/2523-multiple-accordion-implementation-pt-2
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.
🚀
@@ -326,3 +326,9 @@ export interface ValueSet { | |||
} | |||
|
|||
export type ValueSetType = keyof ValueSet; |
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.
ValueSetType looks good to me and is consistent with how it is referred to in the TCR
@@ -326,3 +326,9 @@ export interface ValueSet { | |||
} | |||
|
|||
export type ValueSetType = keyof ValueSet; | |||
|
|||
export const valueSetTypeToClincalServiceTypeMap = { |
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.
I can't think of something better off the top of my head! I think there's a decent chance many things get renamed as we work through tech debt in the coming weeks,
* of valueSetName:author:system and the values are all the value set items that | ||
* share those identifiers in common, structed as a GroupedValueSet | ||
*/ | ||
function groupValueSetsByNameAuthorSystem(valueSetsToGroup: ValueSetItem[]) { |
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.
Nice, makes sense to split this out into its own function!
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.
I'm actually seeing some errors when I try to load the Search page and I'm wondering if they are related to any changes you've made.
Warning: Select elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled select element and remove one of these props. More info: https://reactjs.org/link/controlled-components
at select
at div
at div
at form
at SearchForm (webpack-internal:///(ssr)/./src/app/query/components/SearchForm.tsx:33:27)
at Suspense
at div
at Query (webpack-internal:///(ssr)/./src/app/query/page.tsx:36:82)
at ClientPageRoot (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/client-page.js:14:11)
at Lazy
at InnerLayoutRouter (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:242:11)
at RedirectErrorBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/redirect-boundary.js:73:9)
at RedirectBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/redirect-boundary.js:81:11)
at NotFoundBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/not-found-boundary.js:84:11)
at LoadingBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:347:11)
at ErrorBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/error-boundary.js:158:11)
at InnerScrollAndFocusHandler (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:153:9)
at ScrollAndFocusHandler (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:228:11)
at RenderFromTemplateContext (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/render-from-template-context.js:16:44)
at Lazy
at OuterLayoutRouter (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:367:11)
at Lazy
at InnerLayoutRouter (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:242:11)
at RedirectErrorBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/redirect-boundary.js:73:9)
at RedirectBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/redirect-boundary.js:81:11)
at NotFoundErrorBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/not-found-boundary.js:76:9)
at NotFoundBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/not-found-boundary.js:84:11)
at LoadingBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:347:11)
at ErrorBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/error-boundary.js:158:11)
at InnerScrollAndFocusHandler (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:153:9)
at ScrollAndFocusHandler (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:228:11)
at RenderFromTemplateContext (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/render-from-template-context.js:16:44)
at Lazy
at OuterLayoutRouter (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/layout-router.js:367:11)
at Lazy
at DataProvider (webpack-internal:///(ssr)/./src/app/utils.tsx:70:29)
at Lazy
at div
at body
at html
at RedirectErrorBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/redirect-boundary.js:73:9)
at RedirectBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/redirect-boundary.js:81:11)
at ReactDevOverlay (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/react-dev-overlay/app/ReactDevOverlay.js:87:9)
at HotReload (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/react-dev-overlay/app/hot-reloader-client.js:322:11)
at Router (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/app-router.js:202:11)
at ErrorBoundaryHandler (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/error-boundary.js:112:9)
at ErrorBoundary (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/error-boundary.js:158:11)
at AppRouter (webpack-internal:///(ssr)/./node_modules/next/dist/client/components/app-router.js:564:13)
at Lazy
at r2 (C:\Repos\phdi\containers\tefca-viewer\node_modules\next\dist\compiled\next-server\app-page.runtime.dev.js:39:18701)
at r2 (C:\Repos\phdi\containers\tefca-viewer\node_modules\next\dist\compiled\next-server\app-page.runtime.dev.js:39:18701)
at ServerInsertedHTMLProvider (C:\Repos\phdi\containers\tefca-viewer\node_modules\next\dist\compiled\next-server\app-page.runtime.dev.js:39:24131)
Yes was also getting annoyed by this! Going to solve the slew of frontend console errors in one followup PR |
I'm noticing that the rows for the codes are pretty wide - would it be possible to make them a little narrower, closer to what's in figma? |
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.
Apologies it took me so long to get to this, but looks good aside from Laura's point about wanting to make the rows narrower! Let's get that addressed and make sure we tag her for re-review so she can okay once it's done.
…' of https:/CDCgov/phdi into rob-and-bob/2523-multiple-accordion-implementation-pt-2
@lg-king should be ready for re-review now! |
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 good!
* Adding No results logic for TEFCA CustomizeQuery * commentary
PULL REQUEST
Summary
Implements the frontend for the customize query multiple accordion selection. Follow on from #2564 and takes care of #2521
Inclusion switches work (stuck an extra console log into the handle apply listener)
Screen.Recording.2024-09-17.at.10.1.mp4
Redirect back to main page
Screen.Recording.2024-09-17.at.10.18.33.AM.mov
Related Issue
Fixes #2523
Acceptance Criteria
Once we have a back end function that divides up the returned DB rows by their coding scheme, we want to display this information correctly to the user on the front end. We'll pass in all the relevant value set data to the CustomizeQuery component, but on each tab of labs, medications, and conditions, we want to show one accordion per coding scheme present in the data.
Additional Information
There's a minor UI issue where the accordions don't persist their collapsed state when the tab switches because I couldn't get the out-of-box
handleToggle
function from Truss to work correctly. Didn't want that to block this work, so made a follow up ticket for it.Additionally, did want to ask the team what their thoughts are about some of the transformation functions in
customizeQueryUtils.ts
, since ideally, I'd be writing some unit tests with some known starting / end-state test data. Wondering what our larger state of frontend testing looks like and wrote up a couple questions into a ticket here. Happy to close this if this is duplicated anywhere.