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

fix: make datetime input support multicalendar [DHIS2-17618] #404

Merged
merged 8 commits into from
Oct 18, 2024

Conversation

tomzemp
Copy link
Member

@tomzemp tomzemp commented Oct 11, 2024

See https://dhis2.atlassian.net/browse/DHIS2-17618

Background

This PR makes the datetime input use our DHIS2 calendar component (which will load based on the calendar set in system settings). We decided that for now it was easier to just combine the DHIS2 calendar with a browser-native time picker (rather than the alternative of refactoring the calendar to also incorporate the possibility to select a time). To be honest, since we don't expect this to be used that much, I just tried to go for something functional, not something super designed or performant 😄 .

While working on this, I noticed that valid Ethiopian dates (2016-13-05) were not persisting, which led to a discussion with Jan Bernitt where we clarified that the objective from now onwards (though not necessarily in the past) is that the backend will store ISO dates and that the frontend needs to convert these. Hence I made a bug report for the existing Date input (https://dhis2.atlassian.net/jira/software/c/projects/DHIS2/issues/DHIS2-18219) and also fixed that in the context of this PR.

You can create a data element of type:datetime and then add it to a dataset to look at the changes:
image

Notes

  • The datetime input (like also the date input and radio-boolean input) updates when a selection is made. This seems reasonable as we had sync onBlur for the general input to prevent unnecessary syncing while the user was typing and changing the input, but the datetime input changes on selection. The one downside of this is that the browser-native time input behaves a bit different from browser to browser and chrome will change the value of the time input when you type the first hour part (e.g. if you type 12, chrome sets a time of 12:00 on the input). This leads to some unnecessary syncing, but I think this is acceptable (particularly as we don't expect this to be used and other browsers like Firefox and Opera make you enter a valid time before it sets the value of the input)
    -I've added input.onFocus() to the input as this gives us the proper blue border styling on the input, and similarly have done input.onBlur() after the sync...which is a bit weirder logically, but it also ensures that we get the box to change green on a successful sync.

Testing
I've added a number of automated tests to cover the basic use of the input (covering gregorian/ISO, Ethiopian, and Nepali calendars).
I've also done manual testing (which made me notice the issue of needing to convert to ISO dates 🤪)

@tomzemp tomzemp requested a review from a team October 11, 2024 11:12
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Oct 11, 2024

🚀 Deployed on https://pr-404--dhis2-data-entry.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify October 11, 2024 11:14 Inactive
convertFromIso8601,
convertToIso8601,
} from '@dhis2/multi-calendar-dates'

Copy link
Member

Choose a reason for hiding this comment

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

Should we add this to the multi-calendar library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean should we add the option to return as a string into the existing convertFromIso8601, convertToIso8601 functions ? I think that would be a good idea, but maybe it would be better to do what we need for the data entry app and then make feature requests for multi-calendar-dates? There are a number of things (particularly on the other multi-calendar-support PR #397) that might make sense to move to multi-calendar-date going forward, but I think then we'd want to talk through it more if we export it generally?

@dhis2-bot dhis2-bot temporarily deployed to netlify October 18, 2024 13:15 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 18, 2024 13:51 Inactive
@tomzemp
Copy link
Member Author

tomzemp commented Oct 18, 2024

Note: I noticed after merging that the audit values for date/datetime showed up as ISO (because they're persisted as ISO), so I added a commit to convert this to system calendar (which I think makes more sense as it is what the user sees as the value) (it's still a bit confusing because the real value at the moment is an ISO date)

@tomzemp tomzemp merged commit e8d673d into master Oct 18, 2024
8 checks passed
dhis2-bot added a commit that referenced this pull request Oct 18, 2024
## [100.8.5](v100.8.4...v100.8.5) (2024-10-18)

### Bug Fixes

* make datetime input support multicalendar [DHIS2-17618] ([#404](#404)) ([e8d673d](e8d673d))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.8.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants