feat(Accessibility): Improve accessibility behaviors, make them reusable #247

sophieH29 · 2018-09-18T16:26:16Z

Currently, accessibility behaviors bring few constraints on how they are implemented and used.

Main points:

reusing parts of behaviors: you want to implement ToggleButtonBehaviour attributes based on ButtonBehavior, without copy-pasting same code
behavior can be either an object or function, which brings additional complexity, as you always have to check it. If we gonna make behaviors more agnostic and distribute them for any kind of framework, I think we need to stick to one concrete structure of behaviors to avoid possible issues in future.
Already seen some complexities if to look at SelectableListItemBehavior
const SelectableListItemBehavior: (props: any) => IAccessibilityDefinition = (props: any) => ({
We define it with double function to pass correct props from BasicListBehavior

Solution is to change accessibility behavior definition to one solid structure

export type Accessibility = ((props: any) => IAccessibilityDefinition)

Reusability

Currently I made ButtonBehavior attributes reusable, by exporting it's attributes and using it in ToggleButtonBehavior as

export const ButtonAttributes = (props: any): AccessibilityAttributes => ({
  root: {
    role: props.as === 'button' ? undefined : 'button',
    'aria-disabled': !!props['disabled'],
  },
})

const ButtonBehavior: Accessibility = (props: any) => ({
  attributes: ButtonAttributes(props),
})

const ToggleButtonBehavior: Accessibility = (props: any) => ({
  attributes: {
    root: {
      ...ButtonAttributes(props).root,
      'aria-pressed': !!props['active'],
      'aria-disabled': !!props['disabled'],
    },
  },
})

Other behaviors can be changed like that in case we'll see any other meaningful opportunity to reuse behavior's parts (attributes, keyActions, etc).

codecov · 2018-09-19T07:49:37Z

Codecov Report

Merging #247 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #247   +/-   ##
=======================================
  Coverage   89.54%   89.54%           
=======================================
  Files          62       62           
  Lines        1176     1176           
  Branches      152      175   +23     
=======================================
  Hits         1053     1053           
  Misses        121      121           
  Partials        2        2

Impacted Files	Coverage Δ
src/components/Chat/Chat.tsx	`95.45% <100%> (ø)`	⬆️
src/index.ts	`100% <100%> (ø)`	⬆️
src/components/List/List.tsx	`100% <100%> (ø)`	⬆️
src/components/Icon/Icon.tsx	`83.33% <100%> (ø)`	⬆️
src/components/Image/Image.tsx	`100% <100%> (ø)`	⬆️
src/components/Button/Button.tsx	`95.74% <100%> (ø)`	⬆️
src/components/Button/ButtonGroup.tsx	`97.29% <100%> (ø)`	⬆️
src/components/Menu/Menu.tsx	`100% <100%> (ø)`	⬆️
src/components/List/ListItem.tsx	`94.87% <100%> (ø)`	⬆️
src/components/RadioGroup/RadioGroup.tsx	`95.38% <100%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08706c8...5522990. Read the comment docs.

hughreeling

Nice cleanup. Looks good.

smykhailov · 2018-09-19T08:44:28Z

src/lib/accessibility/interfaces.ts

@@ -178,4 +178,4 @@ export type ActionsKeyHandler = {
 export type KeyboardHandler = (event: KeyboardEvent) => void
 export type EventHandler = (event: Event) => void

-export type Accessibility = IAccessibilityDefinition | ((props: any) => IAccessibilityDefinition)
+export type Accessibility = ((props: any) => IAccessibilityDefinition)


'Accessibility' is a very generic word, should we consider to change it to something more meaningful

yes, quite generic, any ideas of a name?
Currently, we already have IAccessibilityDefinition, IAccessibilityBehavior

what do you think about IAcessibilityProvider?

it's a type, so it will be AccessibilityProvider. Can be like that, we need more opinions and ideas :) @jurokapsiar what do you think on naming?

I personally think Accessibility is ok, if there's concern, than it should probably be AccessibilityBehavior

yeah, for me both are ok.
But guys, let's better merge this PR if the changes are ok for you.
And decide on naming other time, maybe we'll get inspired and find the perfect name :)

mnajdova · 2018-09-21T14:25:36Z

src/lib/accessibility/interfaces.ts

@@ -178,4 +178,4 @@ export type ActionsKeyHandler = {
 export type KeyboardHandler = (event: KeyboardEvent) => void
 export type EventHandler = (event: Event) => void

-export type Accessibility = IAccessibilityDefinition | ((props: any) => IAccessibilityDefinition)
+export type Accessibility = ((props: any) => IAccessibilityDefinition)


nit: can be replaced with:

export type Accessibility = (props: any) => IAccessibilityDefinition

More important, any specific reason why we don't support just object definition, but only functions now? What is the problem of having both?

We could have both object and functions, but as described above, it started to bring complexities which are more seen in the future, here are a couple of reasons:

we always have to check whether we get function or object (as we aim to make accessibility framework agnostic, it can also bring some stress for the users, as they need to implement custom behavior derived from Accessibility type).

If we want to reuse some parts of other behavior, it would be quite unsafely or even impossible to make it nicely

Already seen a not good way of implementation in ListItemBehavior, where we have to use a double function to pass props

And since most of the behaviors (for now like 80-90%) needs props to correctly render aria*-attributes, I don't see a reason to support such ambiguous structure for the rest of 10%

mnajdova · 2018-09-21T14:29:37Z

src/lib/accessibility/Behaviors/List/ListItemBehavior.ts

-const ListItemBehavior: (props: any) => IAccessibilityDefinition = (props: any) =>
-  props.selection ? SelectableListItemBehavior(props) : BasicListItemBehavior
+const ListItemBehavior: Accessibility = (props: any) =>
+  props.selection ? SelectableListItemBehavior(props) : BasicListItemBehavior(props)


To me it feels kind a awkward that all accessibility behaviors are defined as functions but they are starting with capital letter.

From point of view that functions should be lowercase - yes agree with that.
But for me, it seems better as it is now, and I can't explain why :)
If anyone also thinks about changing to lowercase, of course, we can consider it.

mnajdova · 2018-09-21T14:32:07Z

src/lib/renderComponent.tsx

-  )(props)
+  const accessibility: IAccessibilityDefinition = (customAccessibility ||
+    defaultAccessibility ||
+    DefaultBehavior)(props)



The callable util allow us exactly to have objects, or functions that can be invoked for the same Type. See the comment above.

yes - but even from this perspective, now we handle if we get correct type (what is not good) from outside. In my opinion, this should be handled in accessibility functionality. We should treat Accessibility as a separate lib, we just give information to it, and it does everything for us. Anyway, I answered above what were the reasons for such changes :)

mnajdova · 2018-09-21T14:34:36Z

Nice changes, @sophieH29 . Left some comments for consideration, and please fix the linting errors. After reasoning for/resolving the comments, should be good for merge.

# Conflicts: # src/lib/accessibility/Behaviors/Tab/TabListBehavior.ts # src/lib/accessibility/Behaviors/Toolbar/ToolbarBehavior.ts

miroslavstastny · 2018-10-04T08:45:48Z

src/lib/accessibility/Behaviors/Button/ButtonBehavior.ts

-      role: props.as === 'button' ? undefined : 'button',
-      'aria-disabled': !!props['disabled'],
-    },
+export const ButtonAttributes = (props: any): AccessibilityAttributes => ({


How does this help with reusability? If ToggleButtonBehavior is built on top of ButtonBehavior, it can call ButtonBehavior directly:

const parent = ButtonBehavior(props) return { attributes: { root: { ...parent.attributes.root, 'aria-pressed': !!props['active'], 'aria-disabled': !!props['disabled'], }, }, }

Extracting to ButtonAttributes can be understood as only behaviors with extracted attributes can be 'extended' which (1) is not true and (2) limits extending behaviors for library users.

@miroslavstastny agree with your point that it is possible to do that as in your example, although I wouldn't call it "parent". It is like a composition - you can build new behavior with different parts. + in your approach the whole behavior is initialized and we use from it just part, so it can have a bigger performance impact.
Let me know what you think.

Understand, but do we need it now or is it just a premature optimisation? Especially considering that currently the only refactored behavior is ButtonBehavior which has just attributes and nothing else.
I think that your approach is valid when it is required but I would not define it as the only/primary reusability pattern.

The main change in this PR - is to give the behavior a solid structure, so now it can be only a function. With this, we remove ambiguity and make it simpler.
The change in ButtonBehavior is just a bonus.

I think that your approach is valid when it is required but I would not define it as the only/primary reusability pattern. - agree with that, this approach of reusing parts is not a primary pattern

Don't take me wrong, I like the change to functions.
I would just prefer not to have ButtonAttributes

Got your point @miroslavstastny, let's agree then, I will remove ButtonAttributes as this is not really needed right now.

miroslavstastny · 2018-10-04T08:46:43Z

src/lib/accessibility/Behaviors/Button/ButtonGroupBehavior.ts

@@ -5,7 +5,7 @@ import { Accessibility, FocusZoneMode } from '../../interfaces'
 * Adds role 'presentation' to 'root' component's part.
 * Wraps component in FocusZone allowing arrow key navigation through the children of the component.
 */
-const ButtonGroupBehavior: Accessibility = {
+const ButtonGroupBehavior: Accessibility = (props: any) => ({


Behaviors are functions, not objects. Why their names start with capital letters?

From point of view that functions should be lowercase - yes agree with that.
I left as is just from that point how it looks

static defaultProps = { accessibility: MenuBehavior as Accessibility, }

vs

static defaultProps = { accessibility: menuBehavior as Accessibility, }

Marija has same point on that, so then I will change to lowercase

Speaking of as, according to @smykhailov it is no longer necessary after @kuzhelov fixed generated paths in .d.ts.
Another thing is that defaultProps should be typed to components' prop interface :-)

Another thing is that defaultProps should be typed to components' prop interface :-) yeah, I know that
Ok, just to summarize, I'll make behaviors lower case.
I think defaultProps typing should be fixed as separate PR

miroslavstastny

changelog and go!

Sofiya Huts added 3 commits September 18, 2018 18:00

Made improvement to accessibility behaviors

419e58d

Made improvement to accessibility behaviors

d433feb

small fix

e0d676f

sophieH29 added 🚀 ready for review RFC labels Sep 18, 2018

sophieH29 self-assigned this Sep 18, 2018

sophieH29 requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners September 18, 2018 16:26

sophieH29 changed the title ~~feat(Accessibility): Improve accessibility behaviors, make them extendable~~ feat(Accessibility): Improve accessibility behaviors, make them reusable Sep 18, 2018

Merge branch 'master' into feat/accessibility-behaviors-extendable

77b70c9

kolaps33 approved these changes Sep 19, 2018

View reviewed changes

hughreeling reviewed Sep 19, 2018

View reviewed changes

smykhailov reviewed Sep 19, 2018

View reviewed changes

sophieH29 and others added 5 commits September 20, 2018 10:28

Merge branch 'master' into feat/accessibility-behaviors-extendable

fc90e65

Merge branch 'master' into feat/accessibility-behaviors-extendable

45db1d3

Fixes after auto merge

fff32d6

Merge branch 'master' into feat/accessibility-behaviors-extendable

d3aa39a

Merge branch 'master' into feat/accessibility-behaviors-extendable

b65cf6b

mnajdova reviewed Sep 21, 2018

View reviewed changes

Sofiya Huts and others added 4 commits September 21, 2018 17:26

Updates after CR comments

044bbca

Merge branch 'master' into feat/accessibility-behaviors-extendable

653d57f

Merge branch 'master' into feat/accessibility-behaviors-extendable

adcf243

Merge branch 'master' into feat/accessibility-behaviors-extendable

253296a

# Conflicts: # src/lib/accessibility/Behaviors/Tab/TabListBehavior.ts # src/lib/accessibility/Behaviors/Toolbar/ToolbarBehavior.ts

miroslavstastny reviewed Oct 4, 2018

View reviewed changes

sophieH29 and others added 10 commits October 4, 2018 13:36

Merge branch 'master' into feat/accessibility-behaviors-extendable

235e178

fixes after CR comments

022dc05

remove redundant comment line

b4067ed

Rename files to lowercase

d675074

Rename files to lowercase

a6c9e95

Rename files to lowercase

a73f5e8

no message

9d228fd

no message

1090a6a

Fix test

ccd5287

Merge branch 'master' into feat/accessibility-behaviors-extendable

bb90927

miroslavstastny approved these changes Oct 5, 2018

View reviewed changes

Update CHANGELOG

5522990

sophieH29 merged commit e9111be into master Oct 5, 2018

sophieH29 deleted the feat/accessibility-behaviors-extendable branch October 5, 2018 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Accessibility): Improve accessibility behaviors, make them reusable #247

feat(Accessibility): Improve accessibility behaviors, make them reusable #247

sophieH29 commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading

hughreeling left a comment

smykhailov Sep 19, 2018

sophieH29 Sep 19, 2018

smykhailov Sep 20, 2018

sophieH29 Sep 20, 2018

jurokapsiar Sep 21, 2018

sophieH29 Sep 21, 2018

mnajdova Sep 21, 2018

mnajdova Sep 21, 2018

sophieH29 Sep 21, 2018

mnajdova Sep 21, 2018

sophieH29 Sep 21, 2018

mnajdova Sep 21, 2018

sophieH29 Sep 21, 2018

mnajdova commented Sep 21, 2018

miroslavstastny Oct 4, 2018

sophieH29 Oct 4, 2018 •

edited

Loading

miroslavstastny Oct 4, 2018

sophieH29 Oct 4, 2018

miroslavstastny Oct 4, 2018

sophieH29 Oct 4, 2018

miroslavstastny Oct 4, 2018

sophieH29 Oct 4, 2018

miroslavstastny Oct 4, 2018

sophieH29 Oct 4, 2018 •

edited

Loading

miroslavstastny left a comment

feat(Accessibility): Improve accessibility behaviors, make them reusable #247

feat(Accessibility): Improve accessibility behaviors, make them reusable #247

Conversation

sophieH29 commented Sep 18, 2018 • edited Loading

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

hughreeling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Sep 21, 2018

Choose a reason for hiding this comment

sophieH29 Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophieH29 Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

miroslavstastny left a comment

Choose a reason for hiding this comment

sophieH29 commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading

sophieH29 Oct 4, 2018 •

edited

Loading

sophieH29 Oct 4, 2018 •

edited

Loading