feat: implement grouping for numeric data items with legend set [DHIS2-21699] - #340
feat: implement grouping for numeric data items with legend set [DHIS2-21699]#340edoardo wants to merge 16 commits into
Conversation
No reason to branch inside FilterSection, instead treat it in the same way as OptionSetCondition.
In the UI we use the group term, stick to it in the code for clarity.
Instead pass the legendSetIds directly in the rest prop. No reason to change the normalization function adding legendSets and helper function to parse them to extract the ids.
|
🚀 Deployed on https://pr-340.event-visualizer.netlify.dhis2.org |
There was a problem hiding this comment.
This wasn't the easiest to review because this is a combination of new code and reorganised code, which made it a bit hard to tell which parts to focus.
In general I think this is looking very well organised, but I do see two parts that could maybe be aligned a bit more with pre-established patterns we have in the codebase. These two suggestions actually address the "doubts and open questions" you had:
Conditions context
I think we can align this with the other context providers we have in the app, InterpretationsProvider, MetadataProvider, ScrollBox:
- Instead of having a
conditions-context.tsyou could have aconditions-provider. In this file you then define the type(s), the context, a named provider that populates the context-providervalueand the hook - Then
filtering-sectionbecomes much smaller and does not have mixed responsibilities - Instead of trying to capture all of this in review comments I created #348
Seeding the default grouping
Technically the implementation looks perfect to me. I also see why you say "We don't have a similar precedent". We do have thunks and we do have a listener, but what we have already is a listener that is specific to a single slice and it lives there, while this new listener is triggered by actions from different slices. This means we can't add it to a slice.
What I would suggest doing is the following:
- Move the thunk to
src/store/thunks.ts - Create a new file
src/store/listeners.tsand add the listeners there. Not just the one you added in the current PR, but the one currently in the navigation slice too.
Doing this reorganisation is perhaps better done in a separate PR. If you agree it's needed, please create a JIRA ticket.
| const options = useMemo(() => { | ||
| const legends = | ||
| legendSets.find(({ id }) => id === legendSetId)?.legends ?? [] | ||
|
|
||
| return legends.map(({ id, name }) => ({ value: id, label: name })) | ||
| }, [legendSets, legendSetId]) |
There was a problem hiding this comment.
MINOR:
Instead of ?? [] on L44 you can just do legends?.map on L46.
In fact, that means, you can just do an implicit return with everything conditionally chained:
() => legendSets.find(({ id }) => id === legendSetId)?.legends?.map(({ id, name }) => ({ value: id, label: name })) ?? []| dimensionName: dimension.name, | ||
| suffix: dimension.suffix, | ||
| itemsText: chipItemsText, | ||
| isGrouped: Boolean(conditions?.legendSet), |
There was a problem hiding this comment.
MINOR: To align isGrouped with groupingName maybe both should be based on legendSet?
So then isGrouped: Boolean(legendSet) and also add legendSet to the dependency array of the useCalback hook...
| export const canDimensionHaveLegendSets = ( | ||
| dimension: Pick<DimensionMetadataItem, 'dimensionType' | 'valueType'> | ||
| ): boolean => | ||
| dimension.dimensionType === 'PROGRAM_INDICATOR' || |
There was a problem hiding this comment.
Just a question... Are PIs numeric by definition? I saw some (I think pre-existing) sort logic that assumes we are always sorting numbers.
|
3 participants
Implements DHIS2-21699
Description
Implements the grouping functionality for numeric data items that are configured to use a legend set.
The dimension modal shows additional UI where the grouping can be configured.
"No grouping" means a legend set is not used and the filtering works as for any other dimension.
The selectable legend sets are shown as radio cards and when selecting one the legend set's groups (bands) can be filtered using a
Transfercomponent where the legend set's groups (bands) are listed.Notes for the PR review
legend-sets-api.tshas been moved from/components/conditions-modal-content/numeric-condition/to/api/but because it has been heavily changed it doesn't show up as a moved file.It now lives in the shared location because it's used in both the hook in
/components/dimension-modal/grouping-radio/and the thunk that is called when a dimension with legend set is added/dragged from the sidebar to the layout without opening the dimension modal.The
ConditionsTabContentcomponent is much smaller now because with the addition of the Grouping section 2 new components have been added,GroupingSectionandFilteringSection.Previously the filtering UI was implemented in the
ConditionsTabContentcomponent.The
LegendSetConditionhas been implemented similarly to theOptionSetCondition, where we don't look at thevalueTypebut render the component whenconditions.legendSetis present. This means that theNumericConditionthat previously was implementing the legendSet case via the Is one of preset options is now only handling numeric types and it's much simpler.Doubts and open questions
conditions-context.tshas been added to provide the context and theuseConditionshook.Previously was part of
ConditionsTabContentand after the refactor of point 2 above ended up unchanged inFilteringSection.It felt wrong that
useConditionswas exported from a UI component where it's not even used.I'm still unsure about this file tho, the hook needs the context so it can be in the same file, but so far we only have hooks in files that carry their name. This context + hook is a new pattern. We could separate further context provider and hook to keep consistency with the naming and the way hooks are used elsewhere.
seed-default-grouping.tsis a bit particular because it defines a thunk and astartAppListener.We don't have a similar precedent.
I'm wondering if this can be structured differently, ie. put the thunk in thunks (or even use something else than the thunk) and the listener in the visUiConfig slice.
I did ask Claude about this but the thunk needs to access the store and the middleware does not currently handle the extra argument. We need to decide if we want to change this.
Quality checklist
Add N/A to items that are not applicable and check them.
Screenshots
Dimension modal showing the grouping radio cards, default for LL is "No grouping" and for PT is the first available legend set:
"No grouping" filtering options are "Show all values" and the filters work as for other dimensions (notice the lack of "Is one of preset options" in the filter type dropdown):
"Grouping" filtering options are "Show all groups" and the filter shows a Transfer with the legend set's available groups (bands):