Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [DHIS2-15462] Use dhis2 UI calendarinput component in forms #3658

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

alaa-yahia
Copy link
Member

@alaa-yahia alaa-yahia commented May 30, 2024

Implements DHIS2-15462


Description

@dhis2/ui CalendarInput is used when capturing dates in forms.

@alaa-yahia alaa-yahia requested a review from a team as a code owner May 30, 2024 08:46
@alaa-yahia alaa-yahia changed the title Use dhis2 UI calendarinput component in forms feat: [DHIS2-15462] Use dhis2 UI calendarinput component in forms May 30, 2024
Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @alaa-yahia, great job on this PR 👏 !

From capture-app side, I only added a small comment about hiding the default label. Additionally, can you please take a look and fix the failed cypress tests related to the calendar data-test selectors?

From the UI library side, these are the pending features that need to be added before merging this PR:

  1. minimum and maximum support data - feat: support editable input | min & max dates | dd-mm-yyyy format in CalendarInput ui#1504
  2. support for typing/removing a date - feat(calendar input): make input editable ui#1445
  3. support for DD-MM-YYYY format - I couldn't find a ticket/PR for this. Let's sync up for the next step on this.

Thank you!

@alaa-yahia
Copy link
Member Author

alaa-yahia commented Jun 3, 2024

Hey @alaa-yahia, great job on this PR 👏 !

From capture-app side, I only added a small comment about hiding the default label. Additionally, can you please take a look and fix the failed cypress tests related to the calendar data-test selectors?

From the UI library side, these are the pending features that need to be added before merging this PR:

  1. minimum and maximum support data - feat: support min & max dates in CalendarInput ui#1504
  2. support for typing/removing a date - feat(calendar input): make input editable ui#1445
  3. support for DD-MM-YYYY format - I couldn't find a ticket/PR for this. Let's sync up for the next step on this.

Thank you!

Hey,
All the three has work between the ui and multi calendar date library.
The one for editable input you mentioned is an old one.
This is the new one, I will update the name of the pull request when finalising it. I will add a ticket for DD-MM-YYYY, the functionality is already implemented in the multi calendar library pr.

minimum and maximum support data - feat: support min & max dates in CalendarInput ui#1504
support for typing/removing a date - feat: support min & max dates in CalendarInput ui#1504
support for DD-MM-YYYY format - support min & max date, DD-MM-YYYY date format

@alaa-yahia alaa-yahia force-pushed the DHIS2-15462-use-dhis2-ui-calendarinput-component-in-forms branch from 8cd2340 to ec8c228 Compare August 12, 2024 02:02
width={calculatedInputWidth}
{...splittedPassOnProps.input}
<CalendarInput
{...passOnProps}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In passOnProps, there are a lot of old props that are no longer used and it's quite hard to keep track. Is it possible to clean it up a bit and leave only the props that CalendarInput actually uses? You can also clean up unused props from parent components of Date.component.js. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update the code!

Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work, well done @alaa-yahia ! 👏
Let's move it to QA and then discuss in one of our Tuesday syncs if we want to release with alpha or wait for the @dhis/ui stable release.

@superskip
Copy link
Contributor

superskip commented Aug 15, 2024

Just one question regarding validation: The new calendar component is doing validation internally, which makes sense since it's supposed to support exotic calendar types. But as far as I can see the Capture app is still doing its own validation - is it possible to make it use the validation result from the new calendar component instead? I can't see how this can be achieved with the current interface though (https://ui.dhis2.nu/components/calendar#props), but maybe that document is outdated.

@alaa-yahia
Copy link
Member Author

Just one question regarding validation: The new calendar component is doing validation internally, which makes sense since it's supposed to support exotic calendar types. But as far as I can see the Capture app is still doing it's own validation - is it possible to make it use the validation result from the new calendar component instead? I can't see how this can be achieved with the current interface though (https://ui.dhis2.nu/components/calendar#props), but maybe that document is outdated.

Yeah @superskip the document is outdated as I haven't worked on docs yet.
I think changing how validation is returned from useDatePicker would make it possible for parent components to receive validation results. Specifically passing validation result to the object passed to onDateSelect which calls the parent onDateSelect.

Copy link

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully on 2.42,2.41.2,2.40.5,2.39.7 versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants