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

Fixed RTL navigations #775

Merged
merged 1 commit into from
Oct 23, 2017
Merged

Fixed RTL navigations #775

merged 1 commit into from
Oct 23, 2017

Conversation

sag1v
Copy link
Contributor

@sag1v sag1v commented Oct 16, 2017

This is due to #774

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling eb7dbf7 on sag1v:patch-1 into ** on airbnb:master**.

@majapw
Copy link
Collaborator

majapw commented Oct 16, 2017

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?

@sag1v
Copy link
Contributor Author

sag1v commented Oct 16, 2017

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
@majapw do you ask if this line:

Move backward (left) and forward (right) by one day

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 RTL mode the right button is moving backwards and left button moves forwards :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.784% when pulling 126ab2d on sag1v:patch-1 into a448352 on airbnb:master.

@sag1v
Copy link
Contributor Author

sag1v commented Oct 18, 2017

@majapw Any further thoughts about this PR?

Copy link
Collaborator

@majapw majapw left a 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?

After that I think this will be ready to go!

@@ -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');
Copy link
Collaborator

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');
}

@@ -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');
Copy link
Collaborator

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');
}

@ljharb
Copy link
Member

ljharb commented Oct 21, 2017

Also, please rebase on the command line so as to remove any merge commits :-)

@sag1v
Copy link
Contributor Author

sag1v commented Oct 21, 2017

@majapw

Can you also update the Home and End shortcuts?

What kind of update? Home & End are not affected by RTL as far as i know

@sag1v
Copy link
Contributor Author

sag1v commented Oct 21, 2017

@ljharb

Also, please rebase on the command line so as to remove any merge commits :-)

How do i fork my PR and do that?
I've tried git clone https://github.com/airbnb/react-dates.git -b sag1v:patch-1 as mentioned here but got

fatal: Remote branch sag1v:patch-1 not found in upstream origin

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 86.681% when pulling 5e1c9b0 on sag1v:patch-1 into a448352 on airbnb:master.

@ljharb
Copy link
Member

ljharb commented Oct 21, 2017

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.

@majapw
Copy link
Collaborator

majapw commented Oct 22, 2017

@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 fn + left or fn + right, it feels like it might make sense to switch them?

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.

majapw
majapw previously approved these changes Oct 22, 2017
ljharb
ljharb previously approved these changes Oct 22, 2017
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

rebased

@sag1v
Copy link
Contributor Author

sag1v commented Oct 22, 2017

@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 RTL until we can find a way to support fn + Arrows.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2017

I believe on a Mac, fn + left/right is indistinguishable from "home"/"end"?

@sag1v
Copy link
Contributor Author

sag1v commented Oct 22, 2017

@ljharb

I believe on a Mac, fn + left/right is indistinguishable from "home"/"end"?

It seems it's not that different than other brands:

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 86.718% when pulling eb8fef6 on sag1v:patch-1 into 145f33a on airbnb:master.

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