-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(react-calendar-compat): added reverse Date selection #34541
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
base: master
Are you sure you want to change the base?
feat(react-calendar-compat): added reverse Date selection #34541
Conversation
173ae67
to
e39205f
Compare
📊 Bundle size report🤖 This report was generated against 8bae075c2f9a6ad3bec9da488f6c748b7d25628b |
Pull request demo site: URL |
@@ -55,6 +55,14 @@ export const CalendarMultidayDayView = () => { | |||
))} | |||
</Dropdown> | |||
</Field> | |||
<h3>Negative date selection</h3> |
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 example description is a bit unclear. Can we expand on it and add more details?
@@ -174,12 +174,18 @@ export function getDateRangeArray( | |||
workWeekDays = [DayOfWeek.Monday, DayOfWeek.Tuesday, DayOfWeek.Wednesday, DayOfWeek.Thursday, DayOfWeek.Friday]; | |||
} | |||
|
|||
daysToSelectInDayView = Math.max(daysToSelectInDayView, 1); | |||
daysToSelectInDayView = |
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.
Why is there a different logic for DateRangeType.Day
?
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.
I think it makes sense, having a -2 week range would be weird to get. One thing to keep in mind @ValentinaKozlova is if this will work with restricted date ranges. so if we only allow days from may 20 to 25 but our range is -3, what happens if we press 21?
Other than that I agree this approach makes the most sense.
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 works the same way as a direct selection. I checked. Will add tests for that
@@ -174,12 +174,18 @@ export function getDateRangeArray( | |||
workWeekDays = [DayOfWeek.Monday, DayOfWeek.Tuesday, DayOfWeek.Wednesday, DayOfWeek.Thursday, DayOfWeek.Friday]; |
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.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Image Converged 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/Image Converged.Image Layout Fit.normal.chromium.png | 50164 | Changed |
vr-tests-react-components/Positioning 2 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 600 | Changed |
vr-tests-react-components/Positioning.Positioning end.chromium.png | 911 | Changed |
vr-tests-react-components/TagPicker 4 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/TagPicker.disabled - Dark Mode.disabled input hover.chromium.png | 659 | Changed |
vr-tests-react-components/TagPicker.disabled - High Contrast.disabled input hover.chromium.png | 1321 | Changed |
vr-tests-react-components/TagPicker.disabled - RTL.disabled input hover.chromium.png | 635 | Changed |
vr-tests-react-components/TagPicker.disabled.disabled input hover.chromium.png | 678 | Changed |
There were 4 duplicate changes discarded. Check the build logs for more information.
if (daysToSelectInDayView < 0) { | ||
endDate = addDays(getDatePart(date), 1); | ||
startDate = addDays(endDate, daysToSelectInDayView); | ||
} else { | ||
startDate = getDatePart(date); | ||
endDate = addDays(startDate, daysToSelectInDayView); | ||
} |
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.
As mentioned offline, it would be great to add tests for this. Possibly even e2e tests since we are touching v8 code.
Added reverse Date selection.
Previous Behavior
Selection was possible only in one direction
New Behavior
Added support of negative date range selection
Related Issue(s)
#28961