feat: support multiple split layers [DHIS2-19542] #3584
Conversation
…ds to sort [DHIS2-6683] (#3541) Implements DHIS2-6683 Description * Disable text selection while dragging the layer cards * Disable map interactivity while dragging the layer cards The reason for the timeout in onSortEnd is because sometimes when releasing the drag (mouseUp), the css change happened so fast that some text got selected right at mouseUp. Delaying it 100ms seemed to fix that.
…[DHIS2-19716] (#3547) Support both ALL and F_EXTERNAL_MAP_LAYER_PUBLIC_ADD as maps-app admin authorities.
Update "Remove all existing layers to add a split map view." to "Remove all other layers to add a split map view."
BRaimbault
changed the title
BRaimbault
left a comment
BRaimbault
left a comment
There was a problem hiding this comment.
Ready for review.
HendrikThePendric
left a comment
HendrikThePendric
left a comment
There was a problem hiding this comment.
It's a bit of a challenge for me to review a PR of this size in an app I am not super familiar with, but I had a go anyway. In general I think things are looking good. I left a few suggestions in the code which you can take or leave.
I've approved in case you want to go ahead and merge. If you are implementing any of my suggestions you can ask for a re-approval after the codebase has changed.
| const setErrorState = ({ key, msg, tab }) => { | ||
| errors[key] = msg | ||
| if (!errors.tab) { | ||
| errors.tab = tab | ||
| } | ||
| } |
There was a problem hiding this comment.
This code probably works fine, but is very hard to digest. My confusion mainly stems from the the following:
errorsis an object that can have a bunch of fields and one of these istab- The single
tabfield gets populated with a string value, i.e.data,period,orgunits, etc. - In the code there are a lot of
ifclauses (notif/else) and these are based on different arguments (i.e.valueTypeandperiodType) - So now, purely by reading the code and without any knowledge about the context, it looks like this is problematic because we could first encounter a "data-tab-error" and then a "period-tab-error" and the second would then overwrite the
tabfield which makes it look like information is lost.
Possibly this behaviour is intentional, and if this is the case I would like to see a name change here, perhaps lasteErrorTab or something.
Or perhaps it won't happen in practice, and if this is the case, it would be clearer to see some if/else clauses at the parts of the code where you switch between different tab-values.
There was a problem hiding this comment.
The idea is to raise all errors found but since those can be in multiple tabs, we have to select which one to jump to.
The order in which the tab prop gets overriden enforce Data > Period > OrgUnit > Filter > Style.
I will rename/comment to clarify.
| {renderingStrategy === | ||
| RENDERING_STRATEGY_SINGLE && ( | ||
| <div> | ||
| <SegmentedControl | ||
| className={styles.flexRowFlow} | ||
| options={[ | ||
| { | ||
| label: i18n.t( | ||
| 'Choose from presets' | ||
| ), | ||
| value: PREDEFINED_PERIODS, | ||
| }, | ||
| { | ||
| label: i18n.t( | ||
| 'Define start - end dates' | ||
| ), | ||
| value: START_END_DATES, | ||
| }, | ||
| ]} | ||
| selected={periodType} | ||
| onChange={(e) => | ||
| dispatch( | ||
| setPeriodType( | ||
| { value: e.value }, | ||
| true | ||
| ) | ||
| ) | ||
| } | ||
| /> | ||
| </div> | ||
| )} | ||
| {renderingStrategy === | ||
| RENDERING_STRATEGY_TIMELINE && ( | ||
| <div className={styles.periodText}> | ||
| {i18n.t( | ||
| 'Choose period for all timeline layers' | ||
| )} | ||
| </div> | ||
| )} | ||
| {renderingStrategy === | ||
| RENDERING_STRATEGY_SPLIT_BY_PERIOD && ( | ||
| <div className={styles.periodText}> | ||
| {i18n.t( | ||
| 'Choose period for all split layers' | ||
| )} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Consider extracting this to a helper component with a switch
| key={`${period.id}-${layer.id}`} | ||
| index={layers.length - index} | ||
| {...layer} // needs to be before period |
There was a problem hiding this comment.
Can the layer have a key or index that you want to use to override the computed values above? If not, I would simply spread the props first and then define individual ones, which is more typical.
There was a problem hiding this comment.
It could be that I am missing some nuances here which would prevent you from doing this, but I wonder if it could perhaps be possible to switch from the useEffect + useState hooks to a useMemo hook. As far as can tell you are calling setErrors inside two useEffect hooks, so I am thinking that if you add all the combined dependencies form those useEffect calls to the useMemo dependency array, then you should be able to have a single memoized errors variable (or perhaps an object with { isValid, errors }. And subsequently you could have a some useEffect hooks that dispatch things when the validity changes.
Parent PR: #3588 > chore: refactor ThematicDialog to functional component DHIS2-18209
Child PR: #3593 > feat: support multiple timeline layers DHIS2-19542
Implements DHIS2-19542
Description
This PR enables adding multiple split layers to a map.
Feats
src/components/map/SplitView.jsxsrc/components/map/MapView.jsxto pass layers as an array instead of single layersrc/util/helpers.jssrc/components/layers/overlays/LayerList.jsxsrc/components/layers/overlays/AddLayerPopover.jsxsrc/components/edit/thematic/ThematicDialog.jsxandsrc/components/edit/styles/LayerDialog.module.csssrc/components/edit/thematic/ThematicDialog.jsx#L268-L288src/components/periods/RenderingStrategy.jsxsrc/components/edit/thematic/ThematicDialog.jsx#L193-L204src/components/edit/thematic/ThematicDialog.jsx#L111-L117src/components/edit/thematic/ThematicDialog.jsx#L222-L224cypress/integration/layers/thematiclayer.cy.jssrc/components/periods/__tests__/RenderingStrategy.spec.jssrc/components/edit/thematic/validateThematicLayer.jssrc/components/edit/thematic/ThematicDialog.jsx#L308-L312src/actions/map.jssrc/constants/actionTypes.jssrc/reducers/map.jssrc/util/legend.jssrc/components/layers/LayerCard.jsxsrc/components/plugin/LegendLayer.jsxFixes
src/components/map/MapItem.jsxto properly handle 2 map items (DHIS2-19808).Chores
src/components/edit/thematic/validateThematicLayer.js.src/components/periods/CalendarInput.jsxsrc/hooks/useLayersLoader.jsandsrc/components/app/App.jsxQuality checklist
Add N/A to items that are not applicable.
Testing