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

O3-2720: Visit stop time input not working in edit mode #1621

Merged
merged 9 commits into from
Feb 6, 2024

Conversation

sherrif10
Copy link
Member

@sherrif10 sherrif10 commented Jan 25, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

Screenshots

Related Issue

Other

Ticket : https://openmrs.atlassian.net/browse/O3-2720

In the original code visitStartDate and visitStopDate are used to initialize the start and stop dates, respectively. If visitStartDate or visitStopDate is provided, they are used; otherwise, the current date is used. however the time-related fields visitStartTime, visitStartTimeFormat, visitStopTime, visitStopTimeFormat) are always initialized based on the provided or current date. in the fix i ensured consistency for both start and stop date. the code uses newDate() to get current date and time. Let me know your suggestions. cc @chibongho @rbuisson @vasharma05

www_screencapture_com_2024-1-26_01_01.mp4

After
Screenshot from 2024-01-25 22-46-20

@chibongho
Copy link
Contributor

I don't think this fixes the underlying problem. This change seems to change the default start and end time in the form to current date / time. We still want to keep it defaulted to the original start / end time of the visit. The problem is with the minDate and maxDate calculation passed to the <DatePicker>

@sherrif10
Copy link
Member Author

Hello @chibongho , i updated the code accordingly .

@sherrif10
Copy link
Member Author

Hi @denniskigen .Looking forward to hear from you.

@rbuisson rbuisson changed the title 03-2720:Visit stop time input not working in edit mode O3-2720: Visit stop time input not working in edit mode Jan 29, 2024
@denniskigen
Copy link
Member

denniskigen commented Jan 29, 2024

Hey, @sherrif10, thanks for working on this. Is the first video you linked to in the PR description meant to replicate the problem? I'm not sure that it does.

@denniskigen
Copy link
Member

denniskigen commented Jan 29, 2024

Some additional context on the minDate and maxDate props for Carbon datepickers:

DatePicker maxDate

Limits the date selection to any date before the date specified. The string format depends on the
locale specified. For example, the top example below is using the default US date format, and the
one below it is using the same format as the locale prop example. One is setting it as September
1st, and the other is setting it as January 9th.

DatePicker minDate

Works similarly to the maxDate prop.

@sherrif10
Copy link
Member Author

sherrif10 commented Jan 29, 2024

Hello @denniskigen , Yes the first vedio is meant to replicate this issue.Are you suggesting i replace VisitDateTimeField with DatePicker for visit stop date while leveraging use of datePicker from carbon.

@@ -497,8 +497,9 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
return [maxVisitStartDatetime, minVisitStopDatetime];
}, [visitToEdit]);

const visitStartDate = getValues('visitStartDate');
minVisitStopDatetime = minVisitStopDatetime ?? Date.parse(visitStartDate.toLocaleString());
const visitStartDate = getValues('visitStartDate') || new Date();
Copy link
Member

Choose a reason for hiding this comment

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

Why are we optionally calling new Date() here? It looks like its already getting called conditionally higher up when initializing defaultValues.

@sherrif10
Copy link
Member Author

sherrif10 commented Jan 29, 2024

You are right , this pattern ensures that if visitToEdit doesn't have a start or stop datetime, the default value is set to the current date. The shared screen shot is the same.

Comment on lines 500 to 503
const visitStartDateValue = getValues('visitStartDate');
const visitStartDate = visitStartDateValue !== undefined ? visitStartDateValue : new Date();
const minVisitStopDatetimeFallback = Date.parse(visitStartDate.toLocaleString());
minVisitStopDatetime = minVisitStopDatetime || minVisitStopDatetimeFallback;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const visitStartDateValue = getValues('visitStartDate');
const visitStartDate = visitStartDateValue !== undefined ? visitStartDateValue : new Date();
const minVisitStopDatetimeFallback = Date.parse(visitStartDate.toLocaleString());
minVisitStopDatetime = minVisitStopDatetime || minVisitStopDatetimeFallback;
const visitStartDate = getValues('visitStartDate') ?? new Date();
minVisitStopDatetime = minVisitStopDatetime ?? Date.parse(visitStartDate.toLocaleString());

Isn't this the whole change you did?

Copy link
Member Author

Choose a reason for hiding this comment

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

hI @vasharma05 ,Here is an updated screen shot
Screenshot from 2024-01-29 14-25-18

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @denniskigen @vasharma05 . i addressed your comments. let me know your suggestions.

@vasharma05 vasharma05 merged commit daf1f03 into openmrs:main Feb 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants