-
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
O3- 2998 monthly calender in appointment form fixed #1072
Conversation
@mogoodrich Can you please review this PR |
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 |
@mogoodrich I see this PR adds an extra navigation feature that @gabriel090 may not be working on. I like his addition. |
@ojwanganto I've resolved the conflicts. Can you review this.. |
@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 |
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.
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 |
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, thanks @Madhu-mac .... but looks like there are some conflicts to resolve before merging?
Hi @mogoodrich, Can you please checkout this branch. I've resolved the confilcts and tested it locally it works fine..!! |
@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..
|
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.. |
@Madhu-mac you should be able to see the failing tests below... |
@mogoodrich I've added some changes & the test cases are running fine. Can you look into it.. |
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.
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( |
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.
Where is this used?
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.
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)
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.
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 |
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.
is this update necessary?
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.
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
@mogoodrich Is there anything that i need to look into?.. |
@mogoodrich I've commented out |
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 |
No worries, @Madhu-mac , let me know if you need anything else from me to get this merged. |
This reverts commit b6249cc
Unfortunately I am reverting this PR, because as @mccarthyaaron mentioned (see link above) this commit removed the end date picker for reoccurring appointments and therefore broke that functionality: Reverted here: |
This reverts commit b6249cc
Requirements
Summary
This PR Fixes the issue with new enhanced monthly calender view
select past dates.
Screenshots
open_mrs.mp4
Related Issue
#1049