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

Changed the MiniCal component to gray out weekends and holidays #513

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

namanhboi
Copy link
Contributor

@namanhboi namanhboi commented May 2, 2024

Summary

Weekends and Holidays are now grayed out in MiniCal component. You can add new holidays by adding new Holiday objects to the holidays list.

I also changed some css styling to make it clear which day is currently selected and made it so that when you increase or decrease the month, the selected date stays the same while the highlighted date still changes. This was changed because if selected dates are automatically set by increasing or decreasing the month then it would land on disabled days like weekends or holidays.

image
Here, 1st of May is selected

image
Now, if we decrease the month to April, the selected date stays the same and the highlighted date is grayish. This was done because there might be holidays (in this case spring break) when we increase or decrease the month so I wouldn't want these dates to be selectable. Here, you can't click on 1st of April because it's in spring break.

image
Go back to March

This pull request is the first step towards integrating the MiniCal component into date inputs site wide.

I have not yet integrated this changed MiniCal into a modal to replace the Input component with input = "date".

Test Plan

I plan to integrate MiniCal into one of the modals and check if the forms are submitted correctly.

@namanhboi namanhboi requested a review from a team as a code owner May 2, 2024 03:50
@dti-github-bot
Copy link
Member

[diff-counting] Significant lines: 44.

Copy link
Collaborator

@Atikpui007 Atikpui007 left a comment

Choose a reason for hiding this comment

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

This looks good.
The grayed out dates show as expected and the functionality works as expected.
This is a good PR

frontend/src/components/MiniCal/MiniCal.tsx Show resolved Hide resolved
@raissaji
Copy link
Contributor

raissaji commented May 6, 2024

Your code looks good, I don't see/foresee any problems with the implementation. I like the idea behind greying out the date selected when you increase or decrease the month. However, in the case where you switch to a month that has a date greyed out, say April 1st, this doesn't seem intuitive to me because the user would have to switch the date anyway. Wouldn't it make more sense to automatically select the first "allowed" date during each month instead? Great job on the PR and happy to discuss with the team!

@namanhboi
Copy link
Contributor Author

Your code looks good, I don't see/foresee any problems with the implementation. I like the idea behind greying out the date selected when you increase or decrease the month. However, in the case where you switch to a month that has a date greyed out, say April 1st, this doesn't seem intuitive to me because the user would have to switch the date anyway. Wouldn't it make more sense to automatically select the first "allowed" date during each month instead? Great job on the PR and happy to discuss with the team!

Yeah I was thinking the same thing, and thanks for reminding me to look into this later. The datapicker is an component from an external librarys, so I think I would have to override a lot of stuff to make it land on the first available date. I might consider removing the highlighted date when the user switch months all together.

@raissaji raissaji self-assigned this Sep 23, 2024
@raissaji raissaji merged commit ad92c8e into master Sep 23, 2024
6 checks passed
@raissaji raissaji deleted the nd433/grayDatePicker branch September 23, 2024 00:55
@raissaji raissaji restored the nd433/grayDatePicker branch September 23, 2024 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants