Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(RTL): Enable RTL for keyboard handlers #656

Merged
merged 10 commits into from
Dec 21, 2018

Conversation

sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Dec 20, 2018

This PR aims to enable RTL for key handlers, so then when users define keyHandlers in accessibility behaviors, in RTL mode, the keys should be swapped:
Left -> Right
Right -> Left

Good example to check how it works is a Menu with submenus.

@sophieH29 sophieH29 changed the title [WIP] feat(RTL): Enable RTL for keyboard handlers feat(RTL): Enable RTL for keyboard handlers Dec 20, 2018
return keyCombination
})
}

Copy link
Member

Choose a reason for hiding this comment

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

We can move this code to a separate function and test it separately:

const keyCombinations = normalizeKeyCombinations(componentPartKeyAction[actionName], isRtl)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can :) But I think this part of the code is very trivial to move it to separate function and test it because it's mainly key mapping.
I tested that correct action handlers are invoked based on key pressed, and this seems to be an important part.
Also, creating one more function leads to having one more rtl prop dependency, hence getKeyDownHandlers will do nothing about this param but just passing it further.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

Everywhere in Stardust (except styles), we use start and end to be direction-agnostic, but here we keep using left and right and remap it for RTL.
Have you thought about using meta key names (ArrowStart, ArrowEnd) and remap them for both directions accordingly?
Would you consider that more clean or more confusing? @jurokapsiar
Other than that the PR looks good to me.

@sophieH29
Copy link
Contributor Author

For me it feels more confusing, I would say start & end is more about a position, not direction. For this would better make sense to have next and previous. But I don't see a reason to change it now, as it is more an internal logic which is not shown to the clients, all they do is just binding a key to action.

@jurokapsiar
Copy link
Contributor

Yes, we considered this. I agree with @sophieH29. Changing the name for ArrowLeft to ArrowPrev is technically more correct, but very unintuitive especially for consumers that are not aware of RTL. In the case of direction in the components, where you control the interface, I agree that we should have start/end/next/prev. On the other hand, people expect to be able to set keyboard handlers for ArrowLeft or ArrowRight.

@sophieH29 sophieH29 merged commit ed50341 into master Dec 21, 2018
@sophieH29 sophieH29 deleted the feat/rtl-keyboard-handlers branch December 21, 2018 11:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants