-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
🚀 Deployed on https://pr-404--dhis2-data-entry.netlify.app |
convertFromIso8601, | ||
convertToIso8601, | ||
} from '@dhis2/multi-calendar-dates' | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
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) |
## [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))
🎉 This PR is included in version 100.8.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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:
Notes
12
, chrome sets a time of12: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 🤪)