fix(accessibility): apply accessibility handlers with applyAccessibilityKeyHandlers()
#1072
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
Option 2:
|
Codecov Report
@@ Coverage Diff @@
## master #1072 +/- ##
==========================================
+ Coverage 81.89% 81.94% +0.05%
==========================================
Files 701 702 +1
Lines 8554 8580 +26
Branches 1169 1172 +3
==========================================
+ Hits 7005 7031 +26
Misses 1534 1534
Partials 15 15
Continue to review full report at Codecov.
|
…x/acc-prop # Conflicts: # packages/react/src/components/RadioGroup/RadioGroupItem.tsx
| @@ -57,8 +54,6 @@ const getKeyDownHandlers = ( | |||
|
|
|||
| eventHandler && eventHandler(event) | |||
| }) | |||
|
|
|||
| _.invoke(props, 'onKeyDown', event, props) | |||
There was a problem hiding this comment.
This is wrong because it will be applied to each slot, see mini repro:
https://codesandbox.io/s/3kplmnz95q?module=/example.js
When you will click Enter on dots, you will key onKeyDown twice
layershifter
changed the title
applyAccessibilityKeyHandlers()
layershifter
added
🚀 ready for review
🧰 fix
| import { Props } from '../types' | ||
| import { KeyboardHandler, OnKeyDownHandler } from './accessibility/types' | ||
|
|
||
| const applyAccessibilityKeyHandlers = (keyHandlers: OnKeyDownHandler, passedProps: Props) => |
There was a problem hiding this comment.
Is the second argument really always going to be Props? If we want to use this functionality with the slots as well, should we receive here ShorthandValue? I know that the Popup has special handling of the triggerProps, but usually this is not the case...
There was a problem hiding this comment.
Done, it makes sense because we also deal with slots 👍
| keyHandlers: OnKeyDownHandler, | ||
| value: Props | ShorthandValue, | ||
| ) => { | ||
| const slotProps: Props = typeof value === 'object' ? value : {} |
There was a problem hiding this comment.
Let's not ignore the other cases of shorthand value. Let's refactor this in the same way it is done in the factories, something like:
const valIsPropsObject = _.isPlainObject(value)
const valIsReactElement = React.isValidElement(value)
const slotProps =
(valIsReactElement && (value as React.ReactElement<Props>).props) ||
(valIsPropsObject && (value as Props)) ||
{}
Fixes #799.