feat(RTL): Enable RTL for keyboard handlers#656
Conversation
| return keyCombination | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
We can move this code to a separate function and test it separately:
const keyCombinations = normalizeKeyCombinations(componentPartKeyAction[actionName], isRtl)There was a problem hiding this comment.
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.
miroslavstastny
left a comment
There was a problem hiding this comment.
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.
|
For me it feels more confusing, I would say |
|
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. |
This PR aims to enable RTL for key handlers, so then when users define
keyHandlersin accessibility behaviors, in RTL mode, the keys should be swapped:Left->RightRight->LeftGood example to check how it works is a Menu with submenus.