-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fixed RTL navigations #775
Conversation
Changes Unknown when pulling eb7dbf7 on sag1v:patch-1 into ** on airbnb:master**. |
Hi @sag1v, I think we'll have to update the phrases that get read out by the screen reader as well. Do the keyboard shortcuts still make sense with this update? |
Hi @majapw, what do you mean by keyboard shortcuts? Iv'e only updated the logic inside the switch statement of the key press for right and left arrows, if we on RTL then the next focused day is calculated in reversed Edit
Still make sense? if that's what you meant then of course it make sense now, that was the whole point of this PR. the bug is that in |
@majapw Any further thoughts about this PR? |
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.
After playing a bit with the current set-up, this change def makes sense. Can you also update the Home
and End
shortcuts?
- https://github.com/airbnb/react-dates/pull/775/files#diff-883056b4b11263241c339f927e93411bR271
- https://github.com/airbnb/react-dates/pull/775/files#diff-883056b4b11263241c339f927e93411bL292
After that I think this will be ready to go!
src/components/DayPicker.jsx
Outdated
@@ -265,7 +265,7 @@ class DayPicker extends React.Component { | |||
break; | |||
case 'ArrowLeft': | |||
e.preventDefault(); | |||
newFocusedDate.subtract(1, 'day'); | |||
!isRTL ? newFocusedDate.subtract(1, 'day') : newFocusedDate.add(1, '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 our linter rules require this to be:
if (isRTL) {
newFocusedDate.add(1, 'day');
} else {
newFocusedDate.subtract(1, 'day');
}
src/components/DayPicker.jsx
Outdated
@@ -286,7 +286,7 @@ class DayPicker extends React.Component { | |||
break; | |||
case 'ArrowRight': | |||
e.preventDefault(); | |||
newFocusedDate.add(1, 'day'); | |||
!isRTL ? newFocusedDate.add(1, 'day') : newFocusedDate.subtract(1, '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.
if (isRTL) {
newFocusedDate.subtract(1, 'day');
} else {
newFocusedDate.add(1, 'day');
}
Also, please rebase on the command line so as to remove any merge commits :-) |
What kind of update? |
Ah, you’d have to clone your fork and rebase your branch there. No worries tho, we can do it for you prior to merging. |
@sag1v While on keyboards that have a home and end button, i agree that it's not as much of an issue, but on keyboards where the functionality is forced by It might be a minority of keyboards for those accessing the RTL version of the datepicker, so maybe it doesn't make sense and home pointing to beginning of the week/end pointing to the end of the week is fine. I will leave it up to you as to whether is makes sense to implement this or not. |
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.
rebased
@majapw Thanks, i see your point. If there is anyway to determine if a Home or End buttons aren't available then we can write the condition for that. Otherwise i think we should merge this PR to at least support the Arrow keys on |
I believe on a Mac, |
|
This is due to #774