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- 2998 monthly calender in appointment form fixed #1072

Merged
merged 15 commits into from May 8, 2024

Conversation

Madhu-mac
Copy link
Contributor

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

This PR Fixes the issue with new enhanced monthly calender view

  • In this new enhanced monthly calendar view the user can able to go to the future months with the arrow buttons.
  • And it only lets the user to select current and future dates which is the actual requirement needed here. The user should not
    select past dates.
  • The selected date gets highlighted in the calendar for better experience

Screenshots

open_mrs.mp4

Related Issue

#1049

@Madhu-mac
Copy link
Contributor Author

@mogoodrich Can you please review this PR

@mogoodrich
Copy link
Member

I believe @gabriel090 may already be looking at this, I will defer to @gabriel090 for the initial review.

@Madhu-mac
Copy link
Contributor Author

I believe @gabriel090 may already be looking at this, I will defer to @gabriel090 for the initial review.

Sure! Hope to see a good feedback from the review

@ojwanganto
Copy link

ojwanganto commented Apr 2, 2024

@mogoodrich I see this PR adds an extra navigation feature that @gabriel090 may not be working on. I like his addition.
@Madhu-mac - please resolve the conflicts.

@ojwanganto ojwanganto closed this Apr 2, 2024
@ojwanganto ojwanganto reopened this Apr 2, 2024
@Madhu-mac
Copy link
Contributor Author

@ojwanganto I've resolved the conflicts. Can you review this..

@mogoodrich
Copy link
Member

@ojwanganto @Madhu-mac I'm a bit "underwater" right now with some things, but should be able to check out later in the week.

@Madhu-mac
Copy link
Contributor Author

@ojwanganto @Madhu-mac I'm a bit "underwater" right now with some things, but should be able to check out later in the week.

Cool

Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

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

It looks like the dates that can't be clicked on are black and the ones that are clickable are grey. Can we reverse so that the future dates are black and the dates in the past are grey?

Also, I think we this PR includes some other changes from other PRs, would be it be possible to have a clean PR with just the changes that are part of this PR?

@Madhu-mac
Copy link
Contributor Author

It looks like the dates that can't be clicked on are black and the ones that are clickable are grey. Can we reverse so that the future dates are black and the dates in the past are grey?

Also, I think we this PR includes some other changes from other PRs, would be it be possible to have a clean PR with just the changes that are part of this PR?

Yeah! I like the idea as well. will be working on it

@gabriel090
Copy link
Contributor

It looks like the dates that can't be clicked on are black and the ones that are clickable are grey. Can we reverse so that the future dates are black and the dates in the past are grey?

Also, I think we this PR includes some other changes from other PRs, would be it be possible to have a clean PR with just the changes that are part of this PR?

Some changes and modifications have been worked on. We can select the date from the grid and save the appointment on the selected date. Open for feedback Thank you.
appointmentSchedular

Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Madhu-mac .... but looks like there are some conflicts to resolve before merging?

@Madhu-mac
Copy link
Contributor Author

Madhu-mac commented Apr 15, 2024

Hi @mogoodrich, Can you please checkout this branch. I've resolved the confilcts and tested it locally it works fine..!!

@mogoodrich
Copy link
Member

@Madhu-mac are these latest changes ready for review? can you explain what the updates do?

@Madhu-mac
Copy link
Contributor Author

Madhu-mac commented Apr 19, 2024

@Madhu-mac are these latest changes ready for review? can you explain what the updates do?

Yes @mogoodrich its ready for the review & the deliverables are..

  • The code defines a React functional component called MonthlyCalendarView that renders a monthly calendar view.
  • It manages the state of the currently displayed month and the selected date using the useState hook.
  • The component includes event handlers for navigating between months and selecting dates, renders the calendar UI with workload counts

@mogoodrich
Copy link
Member

Thanks @Madhu-mac ! Can you look into the failing tests and once we fix those we likely can merge this in?

@Madhu-mac
Copy link
Contributor Author

Thanks @Madhu-mac ! Can you look into the failing tests and once we fix those we likely can merge this in?

@mogoodrich I've updated the branch relavent to the main branch, If any tests are failing Please let me know I'm sure will fix it and get this PR Ready for Merging..

@mogoodrich
Copy link
Member

@Madhu-mac you should be able to see the failing tests below...

@Madhu-mac
Copy link
Contributor Author

@mogoodrich I've added some changes & the test cases are running fine. Can you look into it..

Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

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

Thanks! I am running the tests now, but, I also just added some questions I have.

@@ -350,6 +352,13 @@ const AppointmentsForm: React.FC<AppointmentsFormProps> = ({
<InlineLoading className={styles.loader} description={`${t('loading', 'Loading')} ...`} role="progressbar" />
);

const updateLocations = uniqBy(
Copy link
Member

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The updateLocations function is defined in the AppointmentsForm component but is not directly used within the component itself. We can use it to optimize performance by fetching and updating the list only when necessary, rather than fetching it every time the component renders.( Its for the future purposes if its relative static)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow 100%, but if this function is not yet used, I'd prefer to remove it until it is actually needed/used.

version: 1.0.30001551
resolution: "caniuse-lite@npm:1.0.30001551"
checksum: 10/3ab880797f2a47ce5e2db38700283219faacbddb4382a730883657b2155240aedda1931aac456bc957f61a41c99e15b42f452e5f68e62272def026fd3bf474a7
version: 1.0.30001612
Copy link
Member

Choose a reason for hiding this comment

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

is this update necessary?

Copy link
Contributor Author

@Madhu-mac Madhu-mac Apr 25, 2024

Choose a reason for hiding this comment

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

Depends, If we wanna cope up with future versions or we can just revert it. Its working same, if we update the version also not much of a problem

@Madhu-mac
Copy link
Contributor Author

@mogoodrich Is there anything that i need to look into?..

@Madhu-mac
Copy link
Contributor Author

Madhu-mac commented Apr 26, 2024

@mogoodrich I've commented out updateLocations fn as you said.. Do i need to make any changes to this PR bfr Merging to the main

@mogoodrich
Copy link
Member

Thanks @Madhu-mac and sorry for the delay... assuming the tests pass (I just triggered them to re-run) this is good to merge, thanks!

@Madhu-mac
Copy link
Contributor Author

Thanks @Madhu-mac and sorry for the delay... assuming the tests pass (I just triggered them to re-run) this is good to merge, thanks!

Thanks for the assist @mogoodrich

@mogoodrich
Copy link
Member

No worries, @Madhu-mac , let me know if you need anything else from me to get this merged.

@mogoodrich mogoodrich merged commit b6249cc into openmrs:main May 8, 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