Feature/download template redesign #342

gqcorneby · 2025-01-22T08:38:26Z

📌 References

Issue: Closes #8694wfy5f https://app.clickup.com/t/8694wfy5f

📝 Implementation

added a section component to reuse styles and collapse function for the form sections
divided fields into sections
rearranged advanced options
updated toggle filterOrgUnits logic. Remove clearing of populate, populate dates, and selectedOrgUnits (more notes in reviewers section)
hide populate section until an orgUnit is selected. Previously depended on the org unit checkBox

🔥 Notes for the reviewer

hide populate section until an orgUnit is selected, otherwise the section is empty
updated behavior of selecting organization unit.
- PREVIOUSLY: when toggled, the following were cleared: selectedOrgUnits, populate, populate dates
- NOW: collapsing implies a visual change not data change, so I removed data updates
I opted to use up down arrows instead of left right as indicated in the task document. Seems to fit more with how the section collapses.
I used the "Organisation Unit" label beside the toggle with an indicator of how many is selected (found it useful while testing so included it for now)

📹 Screenshots/Screen capture

📑 Others

- add sections in download template page - collapsible orgunit section instead of a checkbox - rearrange advanced options

ifoche · 2025-01-22T08:38:30Z

Task linked: CU-8694wfy5f Redesign download template page

tokland

[code-only review] Looking good! just some minor details:

tokland · 2025-01-22T11:15:28Z

src/webapp/components/collapsible-section/Section.tsx

+    iconPos = "right",
+    classProps = {},
+    arrowStyle = "upDown",
+}: SectionProps) => {


That's not wrong at all, but typically we prefer to write props: SectionProps here and unpack in the first line of the component, so the signature stays single-line and more readable (subjective!).

tokland · 2025-01-22T11:17:47Z

src/webapp/components/collapsible-section/Section.tsx

+    arrowStyle?: arrowStyle;
+}
+
+export const Section = ({


I see you preferred to define children explicitly in the props instead of using React.FC. We typically use FC, BUT since it adds always children (that's why some people don't like FC), we lose some control, so let's go with this. Eventually, we can decide on creating our own "FCWithProps" or whatever, but no need to to it here.

tokland · 2025-01-22T11:21:52Z

src/webapp/components/collapsible-section/Section.tsx

+    return (
+        <Paper elevation={elevation} className={`${classes.paper} ${classProps.section}`}>
+            <div
+                className={`${classes.header} ${leftIcon} ${classProps.header} ${isOpen ? "" : classes.noBorder}`}


If I am not mistaken, when leftIcon is null, it will appear "null" in the class list. There are packages for that (classnames), but you can also do this: _.compact([name1, name2, name3]).join(" ")

tokland · 2025-01-22T11:28:22Z

src/webapp/components/collapsible-section/Section.tsx

+                {title}
+                {collapsible && !leftIcon && <CollapsibleToggle isOpen={isOpen} arrowStyle={arrowStyle} />}
+            </div>
+            <div className={`${classProps.content}`} style={{ display: isOpen ? "" : "none" }}>


Whenever possible, we strive for immutable props (memoized components will benefit from that).

tokland · 2025-01-22T11:29:09Z

src/webapp/components/collapsible-section/Section.tsx

+    arrowStyle: "upDown" | "rightLeft";
+}
+
+export const CollapsibleToggle = ({ isOpen, arrowStyle }: CollapsibleToggleProps) => {


It's not used externally, right? we can remove the export.

tokland · 2025-01-22T11:35:45Z

src/webapp/components/template-selector/TemplateSelector.tsx

@@ -81,6 +83,7 @@ export const TemplateSelector = ({
            showLanguage: false,
        }
    );
+    const [showAdvancedOptions, setShowAdvancedOptions] = useState<boolean>(true);


Not wrong, but we don't typically pass a type if TS can infer it.

tokland · 2025-01-22T11:38:19Z

src/webapp/components/template-selector/TemplateSelector.tsx

+        // const isCustomProgram = state.templateType === "custom" && state.type !== "dataSets";
+        // const populate = isCustomProgram && filterOrgUnits;
+        // setState(state => ({ ...state, populate }));
+        // clearPopulateDates();


Typically, commented code should be simply removed, we can use git to go back (We may do this for some temporal/testing code, but not in general)

tokland · 2025-01-22T11:42:04Z

src/webapp/components/template-selector/TemplateSelector.tsx

-                {models.length > 1 && (
+            <Section
+                title={<h3 className={classes.title}>{i18n.t("Template")}</h3>}
+                classProps={{ section: classes.section }}


[immutable props] I think we can simply pass "classes" here (unless we really don't want to pass the other keys that Section uses (header?). If that's the case, we can use useMemo to build the object. We would be using useMemo not because it's a costly operation, but to keep the value/prop immutable.

tokland · 2025-01-22T11:47:24Z

src/webapp/components/template-selector/TemplateSelector.tsx

+                        label={i18n.t("Split data entry tabs by section")}
+                    />
+                )}
+


[not to change] The return JSX expression of this component is huuuge (608-294 lines). That's not your fault, you are just moving already existing parts. So this is a note for the future: whenever a component grows too much, let's abstract conceptual units and create new components. But again, it does not make sense to do it now (it's more fitting for a general refactoring/polishing task)

tokland · 2025-01-22T11:51:19Z

src/webapp/components/collapsible-section/Section.tsx

+    elevation = 1,
+    iconPos = "right",
+    classProps = {},
+    arrowStyle = "upDown",


About the expand/collapse styles, I checked what DHIS2 does, and... it depends:

orgUnits tree (icon: left the name): right/down
maps (icon: top-right): up/down

https://play.im.dhis2.org/stable-2-40-6/dhis-web-maintenance/index.html#/list/organisationUnitSection/organisationUnit

https://play.im.dhis2.org/stable-2-40-6/dhis-web-maps/#/GlCLRPPLsWF

I have no strong opinion on this, so PM to decide.

- update section classProps to more specific classNames - update arrowStyle. Add down_right

…om/EyeSeeTea/Bulk-Load into feature/download-template-redesign

tokland

Just some minor details and we're good to go.

Run yarn localize before a push, to check if there are changes on the i18n files (and the commit then, ofc). There are some changes, I think.

src/webapp/components/collapsible-section/Section.tsx

src/webapp/components/template-selector/TemplateSelector.tsx

tokland

All good! Just added a commit for some useMemo and a comment about i18.

tokland · 2025-01-27T12:51:23Z

i18n/es.po

-msgstr "<Sin valor>"
+#, fuzzy
+msgid "Populate"
+msgstr "Plantilla"


We typically check fuzzy tags in PO files (gettext adds them, meaning: "I think this is the translation for a new literal I found, which is very similar to another existing, but I am not sure"). We review if correct and the remove the line "#, fuzzy".

BUT, as all the po files already contain fuzzy tags (a lot), you may leave this task to another "cleaning" PR, not in this feature. Confirm with your PM.

tokland · 2025-01-27T12:56:10Z

src/webapp/components/template-selector/TemplateSelector.tsx

@@ -286,14 +285,11 @@ export const TemplateSelector = ({
    const showPopulate = !(state.templateType === "custom" && !settings.showPopulateInCustomForms);
    const selected = state.id && state.templateId ? getOptionValue({ id: state.id, templateId: state.templateId }) : "";

-    const subSectionClasses = useMemo(() => ({ sectionHeader: classes.subSectionHeader, sectionPaper: classes.subSection }), [classes]);
+    const subSectionClasses = { sectionHeader: classes.subSectionHeader, sectionPaper: classes.subSection };


[ALREADY COMMITED] But here we do need useMemo to keep it immutable, as it's an object directly passed as a prop (string, numbers, booleans, null, undefined are directly comparable, array/objects/pretty much everything else, not)

gqcorneby added 2 commits January 22, 2025 15:35

download template page redesign

4c2d642

- add sections in download template page - collapsible orgunit section instead of a checkbox - rearrange advanced options

add periods section

3f125d4

gqcorneby requested review from tokland and adrianq January 22, 2025 08:38

tokland reviewed Jan 22, 2025

View reviewed changes

gqcorneby added 3 commits January 24, 2025 15:16

add periods section

3956019

cleanup section and templateselector

bfbee3f

- update section classProps to more specific classNames - update arrowStyle. Add down_right

Merge branch 'feature/download-template-redesign' of https://github.c…

6c03661

…om/EyeSeeTea/Bulk-Load into feature/download-template-redesign

gqcorneby requested a review from tokland January 24, 2025 07:29

tokland reviewed Jan 24, 2025

View reviewed changes

add buildClassNames, clean up, prettify

7d1a064

gqcorneby requested review from tokland January 27, 2025 03:55

gqcorneby and others added 2 commits January 27, 2025 12:29

cleanup help icon

b8b34e7

Use useMemo to keep subSectionClasses immutable

8704405

tokland approved these changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/download template redesign #342

Feature/download template redesign #342

gqcorneby commented Jan 22, 2025 •

edited by tokland

Loading

ifoche commented Jan 22, 2025

tokland left a comment •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025

tokland Jan 22, 2025

tokland Jan 22, 2025

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland left a comment •

edited

Loading

tokland left a comment

tokland Jan 27, 2025 •

edited

Loading

tokland Jan 27, 2025 •

edited

Loading

Feature/download template redesign #342

Are you sure you want to change the base?

Feature/download template redesign #342

Conversation

gqcorneby commented Jan 22, 2025 • edited by tokland Loading

📌 References

📝 Implementation

🔥 Notes for the reviewer

📹 Screenshots/Screen capture

📑 Others

ifoche commented Jan 22, 2025

tokland left a comment • edited Loading

Choose a reason for hiding this comment

tokland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tokland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tokland Jan 22, 2025

Choose a reason for hiding this comment

tokland Jan 22, 2025

Choose a reason for hiding this comment

tokland Jan 22, 2025

Choose a reason for hiding this comment

tokland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tokland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tokland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tokland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tokland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tokland left a comment • edited Loading

Choose a reason for hiding this comment

tokland left a comment

Choose a reason for hiding this comment

tokland Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

tokland Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

gqcorneby commented Jan 22, 2025 •

edited by tokland

Loading

tokland left a comment •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland Jan 22, 2025 •

edited

Loading

tokland left a comment •

edited

Loading

tokland Jan 27, 2025 •

edited

Loading

tokland Jan 27, 2025 •

edited

Loading