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

feat(bindings): add useAccessibility hook #1980

Merged
merged 29 commits into from
Jan 8, 2020
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Sep 27, 2019

🍴 useAccessibility()

This PR adds support for useAccessibility() hook that allows to use existing or created behavior in custom components.

πŸ‘¨β€πŸ’» Example

  const getA11Props = useAccessibility(imageBehavior, {
    mapPropsToBehavior: () => ({
      disabled: props.disabled
    }),
    actionHandlers: {
      click: (e: React.KeyboardEvent<HTMLDivElement>) => {
        if (onClick) onClick(e);
      }
    }
  });
  • mapPropsToBehavior() - passes props to behavior, allows to pass props strictly, similar to mapPropsToState() in useStateManager()
  • actionHandlers - works as existing actionHandlers in our components and allows to bind component actions to actions described in behaviors
  • getA11Props(slotName, slotProps) - a function that applies accessibility props and performs proper merge with user's props

πŸ“‘ POC

https://codesandbox.io/s/dropdown-prototype-r3ux9
(prototype of Dropdown component that uses useStateManager() and useAccessibility())


Questions

1️⃣ Refactor to .root() syntax

During our discussion @levithomason, he proposed more fancy signature for getA11Props():

getA11Props('root', someProps) // existing
getA11Props.root(someProps) // proposed

Key issue with this proposal that I don't understand there is Π΅Ρ€Ρƒ replacement of behaviors. For example, I can replace imageBehavior with emptyBehavior without definitions for any slots. How I will not that I need to create a getA11Props.image()?

As solution, I can use Proxy, but it's not supported in IE11 πŸ’£

2️⃣ FocusZone

There is no support in useAccessibility() for FocusZone, I propose to ship it later. Now, I am not fully sure around approaches and want to discuss them separately.

@layershifter layershifter changed the title Feat/use accessibility feat(bindings): add useAccessibilityBehavior Sep 27, 2019
@layershifter layershifter changed the title feat(bindings): add useAccessibilityBehavior feat(bindings): add useAccessibilityBehavior hook Sep 27, 2019
type UseAccessibilityOptions<Props> = {
actionHandlers?: AccessibilityActionHandlers
debugName?: string
mapPropsToBehavior?: () => Props
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be Props | () => Props for API simplification? looking at the unit tests I think passing an object there should be sufficient for most use cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that you mean, but I want to keep it obvious for now and in sync with useStateManager(). We can improve this part once we will receive feedback for this thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that Props | () => Props maybe make sense, in most of the cases I see this as a simple props mapping.. Anyways, not a blocker :)

@layershifter layershifter changed the title feat(bindings): add useAccessibilityBehavior hook feat(bindings): add useAccessibility hook Oct 2, 2019
@layershifter layershifter force-pushed the feat/use-accessibility branch from 50e922c to 6eaee19 Compare October 3, 2019 13:25
@layershifter layershifter force-pushed the feat/use-accessibility branch from fec7a66 to ffbd363 Compare October 4, 2019 13:08
@layershifter layershifter force-pushed the feat/use-accessibility branch from ffbd363 to e7d88da Compare October 4, 2019 13:12
@layershifter layershifter force-pushed the feat/use-accessibility branch from e7d88da to 48d50c0 Compare October 4, 2019 13:23
@@ -1,6 +1,6 @@
import { KeyCombinations } from '@stardust-ui/accessibility'
// @ts-ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

There are no typings for keyboard-key 😭

@ecraig12345 ecraig12345 mentioned this pull request Dec 6, 2019
…b.com/stardust-ui/react into feat/use-accessibility

# Conflicts:
#	packages/accessibility/src/types.ts
#	packages/react-bindings/src/accessibility/shouldHandleOnKeys.ts
#	packages/react/src/components/Menu/Menu.tsx
#	packages/react/src/components/Popup/Popup.tsx
#	packages/react/src/components/Portal/Portal.tsx
#	packages/react/src/components/Tooltip/Tooltip.tsx
#	packages/react/src/components/Tree/Tree.tsx
#	packages/react/src/components/Tree/TreeItem.tsx
#	packages/react/src/lib/getKeyDownHandlers.ts
#	packages/react/src/lib/renderComponent.tsx
@ecraig12345
Copy link
Member

Hi, just a heads up that I made a change #2153 renaming all lib folders to utils. So the changes to lib files in this PR will need to be moved to the new location.

…b.com/stardust-ui/react into feat/use-accessibility

# Conflicts:
#	packages/react/src/components/Tooltip/Tooltip.tsx
#	packages/react/src/utils/createComponent.tsx
#	packages/react/src/utils/factories.ts
@@ -321,7 +217,32 @@ const renderComponent = <P extends {}>(
}
}

return result
if (accessibility.focusZone && accessibility.focusZone.mode === FocusZoneMode.Wrap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract this in a separate function


if (slotHandlers) {
const onKeyDown = (e: React.KeyboardEvent, ...args: any[]) => {
const keyHandlers = definition.keyHandlers[slotName]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const keyHandlers = definition.keyHandlers[slotName]
const keyHandlers = slotHandlers

@layershifter
Copy link
Member Author

Points of confusion / Parts that were blamed

  • getA11Props() - it's not clear that this function is responsible for merging accessibility and client props
  • mapPropsToBehavior() - should it be just an object? How to handle overrides case?
  • childBehaviors - are not implemented as there is a strict contract from child components (accept accessibility prop)

Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

We reviewed and agreed that there are parts that can be improved (see #1980 (comment)), but it can be merged as is now, and we'll iterate later on them. Good job @layershifter!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants