dhis2 / ui

Components and related resources for the DHIS2 design system
https://ui.dhis2.nu
BSD 3-Clause "New" or "Revised" License
40 stars 15 forks source link

refactor: improve performance of calendar component #1557

Closed kabaros closed 2 months ago

kabaros commented 3 months ago

Implements LIBS-440

Some minor performance improvements for multi-calendar component:

Additionally, I've tried to change the component to being semi-controlled, so it doesn't call the hook multiple times unnecessary, and only when leaving the input. This worked in improving the performance (screenshots attached), but overcomplicated the interaction with the inner calendar. After several attempts, I decided to let go of that route in favour of improving the hook in multi-calendar itself - Ticket here.

controlled semi-controlled
Screenshot from 2024-06-10 18-56-04 Screenshot from 2024-06-10 18-56-15
dhis2-bot commented 3 months ago

🚀 Deployed on https://pr-1557--dhis2-ui.netlify.app

alaa-yahia commented 2 months ago

@kabaros : looks good! Sounds good to improve the performance within multi-calendar-dates, e.g. both with memoization and some less expensive validation approaches.

I did like the previous set up of using component from within and feel that's probably easier to make sure styling stays consistent, but yes, better to avoid the unnecessary hook calls, particularly if they're expensive.

When I looked at this, I feel like I had some comments that apply more to the original PR, so I'll also add them here and let you and @alaa-yahia decide what to do with them.

PS @alaa-yahia: great job in adding all of this in 👏!

Date Formatting It's really cool that the format can change as you type dates (e.g. I type 03-12-2020 and now the date is dd-mm-yyyy. However, I think that's likely to be error prone (particularly with mm-dd-yyyy becoming more common when people use English, even outside the US). I think it would be better to enforce the format specified on the CalendarInput (and default that to yyyy-mm-dd). When I tested, it didn't seem that the format was enforced/passed to validators, so it seems like regardless of what you specified, it would always try to guess between dd-mm-yyyy and yyyy-mm-dd formats. Not allowing multiple formats seems like it would also make it easier to speed up/simplify validation by immediately rejecting some strings.

Validation I feel like it would be better to not have this property take a string "warning" || "error" but instead maybe change the property to something like a boolean, e.g. strictValidation and if it's not true use the warning-level validations.

Documentation Would be nice to update the documentation before merging in the main PR. I definitely would not guess some of the behavior (e.g. that possible strings that validation property can take)

Styling Some of the styling when the Clear button is included is a bit "off": image I would definitely tackle this in another issue as I think there are already some undesirable behavior there (e.g. when you specify a width)

Storybook Very minor comment: but it would be nice to change the label (not use ooo in the storybook story and maybe also specify there what the min/max date range are). Also making the story clearable would be nice.

Hey @tomzemp. Thanks for reviewing!. Your suggestions about date formatting & validation makes a lot of sense!. I will apply them after this PR get merged.