refactor: improve performance of calendar component

[kabaros]

Implements LIBS-440

Some minor performance improvements for multi-calendar component:

• use calendar inner components directly to avoid double calls to multi-calendar library.
  • revert Calendar component to its old state, since it doesn't need to handle two states (when used independently, and when used as part of CalendarInput)
• use validation, error, and warning directly from the hook rather than wrapping them in an effect

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

[dhis2-bot]

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

[tomzemp]

@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":

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.

[tomzemp]

I think you need to pass isValid: pickerResults.isValid here? Right now, the cells are not being highlighted if selected (because you're setting selectedDate with logic referencing calendarProps.isValid).

I assume this might also be the cause of the cypress test failures?

[tomzemp]

it would be nice if these were translated...I think that just wrapping the strings within the error throw in multi-calendar-dates should work? (maybe not in dev mode based on when d2-i18n gets imported/i18next gets initialized, but probably in production)

(I get that this comment applies to multi-calendar-dates, but just pointing that out)

Also the label for this input field above Pick a date (not part of the changes, but would be nice to fix) is not wrapped in i18n.t()

[alaa-yahia]

@kabaros @tomzemp I think it would be better if we removed the default "Pick a date" label and instead allow the consumer component to pass a custom label if needed. This change would solve the current issue where InputFields that don't require a label must explicitly pass label="" to remove the default "Pick a date" text.
What do you think?

[tomzemp]

In principle, I'd say that we should keep "Pick a date" in case people are already consuming the component, so they do not update and find that the label has changed, but of our core apps there is maybe only Data Entry (Beta) which is using the default label. Possibly (but not super likely), there are third-party apps using the default label with this component.

I'd be fine with removing the "Pick a date" if there's no objections from @kabaros

[tomzemp]

I'm approving, so as to not block, but think the comment about passing pickerResults.isValid to calendarProps should be addressed before merging. My other comments are all just comments.

[alaa-yahia]

@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": 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.

[alaa-yahia]

LGTM!