(fix) O3-3634: DatePicker input should clear when unspecified is checked #354
Conversation
| @@ -97,6 +107,7 @@ const DateField: React.FC<FormFieldProps> = ({ question, onChange, handler, prev | |||
| <div className={styles.datePickerSpacing}> | |||
| <Layer> | |||
| <OpenmrsDatePicker | |||
| key={key} | |||
There was a problem hiding this comment.
Could we use the field id as the key instead?
There was a problem hiding this comment.
@arodidev When using a dynamic key like field.value, the component may not behave as expected in some cases. So using the original approach which also ensure the component updates correctly when unspecified is check works well with tests as well
There was a problem hiding this comment.
The question id is not dynamic, and would be much more suited rather than the int here
There was a problem hiding this comment.
The major issue is with how the OpenmrsDatePicker propagates the states from the main component to the DatePickerInput component in esm core, when the value is cleared it never trickles down cc @samuelmale
We can report this bug but doing this as well will also re-render the component thus clearing the input when unspecified is check
There was a problem hiding this comment.
@ibacher any thoughts on the above?
|
@ODORA0 did you see the CI failure? |
| if (unspecifiedField.value) { | ||
| setFieldValue(question.id, null); | ||
| setTime(null); | ||
| setKey(prevKey => prevKey + 1); |
There was a problem hiding this comment.
Why do we use an int as key as increment it?
| @@ -97,6 +107,7 @@ const DateField: React.FC<FormFieldProps> = ({ question, onChange, handler, prev | |||
| <div className={styles.datePickerSpacing}> | |||
| <Layer> | |||
| <OpenmrsDatePicker | |||
| key={key} | |||
There was a problem hiding this comment.
The question id is not dynamic, and would be much more suited rather than the int here
brandones
changed the title
|
@ODORA0 @samuelmale @NethmiRodrigo Is it clear what needs to happen before we can put this in? I've fixed the merge conflicts. Looks like CI is passing. Can someone please file a ticket for the bug where the datepicker doesn't update when its value is cleared? |
|
@ODORA0 @brandones I think we might need to change the whole updating the id to force re-render following the fix that @ibacher made in this PR. I made a separate PR to update the datepicker but trying to get the tests fixed before that |
|
@NethmiRodrigo does that PR supersede this one? If so please close this PR and link O3-3634 to your PR. |
|
@brandones doesn't exactly supersede, I just updated the onChange function (updated a prop), but I could bundle in the fix for this in that PR |
|
@NethmiRodrigo if the solution in this PR will still be the right solution, then just merge yours and then we'll merge this PR. If this PR no longer has anything useful about it, just add the solution to your PR and close this one. |
|
(or open a new PR, however you like) |
|
Closing this because it was addressed here. |
Requirements
Summary
Screenshots
Screen.Recording.2024-07-24.at.11.38.06.mov
Related Issue
https://openmrs.atlassian.net/browse/O3-3634
Other