-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Allows selecting the end date first for unattached vertically oriented datepickers #121
Conversation
2f63852
to
71f9c88
Compare
71f9c88
to
b53bed3
Compare
|
||
if (!startDate && orientation === VERTICAL_ORIENTATION && !disabled) { | ||
// Since the vertical datepicker is full screen, we never want to focus the end date first | ||
if (!startDate && withFullScreenPortal && !disabled) { |
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.
do we want to keep this as semver-minor by leaving the vertical orientation check in place?
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.
well but the old behavior was like, broken. So I think this needs to stay.
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 pending a test and a rebase
@@ -894,12 +894,12 @@ describe('DateRangePicker', () => { | |||
}); | |||
}); | |||
|
|||
describe('props.orientation = VERTICAL_ORIENTATION', () => { | |||
describe('props.withFullScreenPortal is truthy', () => { |
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.
This is the test @ljharb
This was behavior from back when we assume the vertical orientation would only ever be used on mobile devices and with a portal. This fixes the assumption.
to: @airbnb/webinfra