-
Notifications
You must be signed in to change notification settings - Fork 234
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) O3-2693 Appointment Creation Form DatePicker uses incorrect minDate format #919
Conversation
...esm-appointments-app/src/appointments/forms/create-edit-form/appointments-form.component.tsx
Show resolved
Hide resolved
Size Change: +11 B (0%) Total Size: 2.84 MB ℹ️ View Unchanged
|
Based on the available guidance for the maxDate prop, it looks like the maxDate and minDate values default to using the US date format, and that the value depends on whatever the locale prop is set to. |
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.
LGTM
@@ -90,7 +90,7 @@ const AppointmentForm: React.FC<AppointmentFormProps> = ({ appointment, patientU | |||
); | |||
|
|||
const appointmentService = services?.find(({ uuid }) => uuid === patientAppointment.serviceUuid); | |||
const today = dayjs().startOf('day').format(); | |||
const today = dayjs().startOf('day').format('DD/MM/YYYY'); |
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.
Adding a quick comment without knowing much about this ticket or the broader tickets, which I think are meant to deal with this broadly. but the display format of a date should be customizable by implementation. For our default in O2, we tend to use a format "1-Jan-2024" , which the month specifically implemented as text so that "DD/MM/YYYY" is not misintrepreted as "MM/DD/YYYY" format by US users. But you may already be on top of that... :) Again, I realize this is a quick fix just to get this working again.
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.
We can directly pass the date object in the maxDate
prop, instead of the format, which is more reliable.
CC: @ibacher @denniskigen
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.
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.
Oh... yes, if the use of this variable is to populate the maxDate
prop, we shouldn't be doing any formatting with it at all, as that will be pretty inherently non-portable.
Requirements
Summary
As of today (2024-01-03) the Appointment creation DatePicker only allows for dates after 2024-04-20
With this change, the minDate defaults correctly to today's date (2024-01-03)
The Carbon documentation is lacking on what format should be accepted for its minDate and maxDate params. From cursory trial-and-error, it seems like the format should follow the
dateFormat
passed in to the DatePicker.https://react.carbondesignsystem.com/?path=/docs/components-datepicker--overview#datepicker-mindate
Screenshots
Related Issue
This is a quick fix for this particular issue, but there are likely other similar problems elsewhere in the code base. A more complete fix should probably involve a standardized DatePicker. See:
https://openmrs.atlassian.net/browse/O3-998
https://openmrs.atlassian.net/browse/O3-1991
Other