feat(docs): adding accessibility description for behaviors #181

kolaps33 · 2018-09-04T13:08:14Z

Adding description for behaviors

moving description form component to the particular behavior
adding new description for behaviors (which been missing)
adding link on the component page pointing into the default description
goal of this PR should have uniform description in some 'regex' way. Descr. should be reused in generation for unit tests, then:
https://github.com/stardust-ui/react/compare/feat/365417-behavior-unit-test

…ior from component

…escription

codecov · 2018-09-04T13:09:08Z

Codecov Report

Merging #181 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #181   +/-   ##
=======================================
  Coverage   91.53%   91.53%           
=======================================
  Files          61       61           
  Lines        1028     1028           
  Branches      136      136           
=======================================
  Hits          941      941           
  Misses         83       83           
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Image/Image.tsx	`100% <ø> (ø)`	⬆️
src/components/Button/Button.tsx	`95.83% <ø> (ø)`	⬆️
src/components/Icon/Icon.tsx	`100% <ø> (ø)`	⬆️

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 2cfa283...5ae23d1. Read the comment docs.

alinais · 2018-09-04T15:10:59Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

 class ComponentDocTag extends React.Component<any, any> {
  getTagDescription = (forTag, fromInfo) => {
    const tags = (fromInfo.docblock && fromInfo.docblock.tags) || []
    return _.result(_.find(tags, 'title', forTag), 'description')
  }

+  getBehaviorURLandName = fromInfo => {
+    const defaultBeharior = {


defaultBeharior -> defaultBehavior

Remove defaultBehavior completely. Instead of returning {name: undefined, url: undefined} return just undefined.

mnajdova · 2018-09-04T16:18:47Z

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

+/**
+ * @description
+ *  Adds role='button' if element type is other than 'button'. This allows the screen readers handle component as button.
+ *  Adds attribute 'aria-disabled=true' based on the property 'disabled'.


I would change ' This allows the screen readers handle component as button.' to 'This allows the screen readers to handle the component as button.'

mnajdova · 2018-09-04T16:19:20Z

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

+/**
+ * @description
+ * Adds role='button' if element type is other than 'button'. This allows the screen readers handle component as button.
+ * Adds attribute 'aria-pressed=true' based on the property 'active'. Based on this, screen readers can recognize state of button, if it is pressed or not.


Same as above.

mnajdova · 2018-09-04T16:23:11Z

src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts

+ *  Adds role 'presentation' on the root element
+ *  Adds role 'menuitem' on anchor element
+ *  Adds attribute 'aria-expanded=true' on anchor element based on "submenuOpened" property
+ *  The behavior is designed for particular structure of menu item. The item consist of root element and anchor inside the root element.


Typo: 'The item consists of root element and anchor inside the root element.'

miroslavstastny · 2018-09-05T08:21:04Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

 class ComponentDocTag extends React.Component<any, any> {
  getTagDescription = (forTag, fromInfo) => {
    const tags = (fromInfo.docblock && fromInfo.docblock.tags) || []
    return _.result(_.find(tags, 'title', forTag), 'description')
  }

+  getBehaviorURLandName = fromInfo => {
+    const defaultBeharior = {


Remove defaultBehavior completely. Instead of returning {name: undefined, url: undefined} return just undefined.

miroslavstastny · 2018-09-05T08:21:34Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

+    return defaultBeharior
+  }
+
+  getDefaultBehavior(info) {


Not all code paths return a value.

Not fixed yet

miroslavstastny · 2018-09-05T08:26:12Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

+
+  getDefaultBehavior(info) {
+    const defaultBehavior = this.getBehaviorURLandName(info)
+    if (defaultBehavior && defaultBehavior.name && defaultBehavior.url) {


If you decide that getBehaviorURLandName returns undefined, the rest of the condition can be safely removed.

miroslavstastny · 2018-09-05T08:27:13Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

 class ComponentDocTag extends React.Component<any, any> {
  getTagDescription = (forTag, fromInfo) => {
    const tags = (fromInfo.docblock && fromInfo.docblock.tags) || []
    return _.result(_.find(tags, 'title', forTag), 'description')
  }

+  getBehaviorURLandName = fromInfo => {


What about adding typings at least for the return value?

getBehaviorURLandName = (fromInfo): {name: string, url: string} | undefined => { ...

+nit: change capitalisation to getBehaviorUrlAndName?

miroslavstastny · 2018-09-05T08:28:51Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

-    )
+    const accDescription = this.getTagDescription(tag, info)
+    const description =
+      accDescription || this.getBehaviorURLandName(info).name ? (


You are calling getBehaviorURLandName() twice in render() (The second call is in getDefaultBehavior())

jurokapsiar · 2018-09-05T09:06:28Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx


 const headerStyle = {
  whiteSpace: 'pre-line',
 }

+type defaultBehavior = {


interface DefaultBehavior { ...

miroslavstastny · 2018-09-05T09:08:02Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

@@ -7,59 +7,67 @@ const headerStyle = {
  whiteSpace: 'pre-line',
 }

-type behaviorFiltered = {
+type defaultBehavior = {


Type names start with capital letter

miroslavstastny · 2018-09-05T09:09:18Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

+    return defaultBeharior
+  }
+
+  getDefaultBehavior(info) {


Not fixed yet

miroslavstastny · 2018-09-05T12:39:05Z

CHANGELOG.md

@@ -25,6 +25,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Adding `behaviors` section to the menu, under the components @kolaps33 ([#119](https://github.com/stardust-ui/react/pull/119))
 - Add `avatar` prop to `Chat.Message` subcomponent @Bugaa92 ([#159](https://github.com/stardust-ui/react/pull/159))
 - add `iconOnly` prop to `Button` @mnajdova ([#182](https://github.com/stardust-ui/react/pull/182))
+- Adding accessibility description for behaviors @kolaps33 ([#181](https://github.com/stardust-ui/react/pull/181))


nit: This should go to Documentation section :-)

kuzhelov · 2018-09-05T21:07:58Z

CHANGELOG.md

 - Add `avatar` prop to `Chat.Message` subcomponent @Bugaa92 ([#159](https://github.com/stardust-ui/react/pull/159))
 - add `iconOnly` prop to `Button` @mnajdova ([#182](https://github.com/stardust-ui/react/pull/182))
 - Add Label `image` and `imagePosition`, removed `onIconClick` prop  @mnajdova ([#55](https://github.com/stardust-ui/react/pull/55/))

+### Documentation
+- Adding `behaviors` section to the menu, under the components @kolaps33 ([#119](https://github.com/stardust-ui/react/pull/119))


for the sake of consistency, lets use present indefinite for these entries (e.g. 'Add behaviors section ...')

kuzhelov · 2018-09-05T21:35:30Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

 class ComponentDocTag extends React.Component<any, any> {
  getTagDescription = (forTag, fromInfo) => {
    const tags = (fromInfo.docblock && fromInfo.docblock.tags) || []
    return _.result(_.find(tags, 'title', forTag), 'description')
  }

+  findDefaultBehaviorInfo = (fromInfo): DefaultBehaviorInfo => {


nit: given fromInfo arg name, it seems that name like fetchDefaultBehaviorInfo will fit better here. Still, please, feel free to reason by yourself whether it should be changed :)

kuzhelov

when trying yarn start, it fails to me. Lets take a look on that tomorrow

…b.com/stardust-ui/react into feat/356643-acc-behaviour-description

kuzhelov · 2018-09-10T08:46:09Z

have checked with the latest branch - there is no yarn start issue anymore on my machine.

kuzhelov · 2018-09-10T08:55:38Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

 class ComponentDocTag extends React.Component<any, any> {
  getTagDescription = (forTag, fromInfo) => {
    const tags = (fromInfo.docblock && fromInfo.docblock.tags) || []
    return _.result(_.find(tags, 'title', forTag), 'description')
  }

+  findDefaultBehaviorInfo = (fetchDefaultBehaviorInfo): DefaultBehaviorInfo => {


the argument should rather be called by noun, componentInfo:

rename fetchDefaultBehaviorInfo -> componentInfo

kuzhelov · 2018-09-10T08:57:02Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

 class ComponentDocTag extends React.Component<any, any> {
  getTagDescription = (forTag, fromInfo) => {
    const tags = (fromInfo.docblock && fromInfo.docblock.tags) || []
    return _.result(_.find(tags, 'title', forTag), 'description')
  }

+  findDefaultBehaviorInfo = (fetchDefaultBehaviorInfo): DefaultBehaviorInfo => {
+    const accessibilityProperty = fetchDefaultBehaviorInfo.props.find(


probably, accessibilityDescription would be a bit more descriptive name for this accessibilityProperty variable

kuzhelov · 2018-09-10T09:00:39Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

+      return undefined
+    }
+
+    const accPropertyFormatted = accessibilityProperty.defaultValue.split('.').pop() + '.ts'


this variable should be better renamed as well, as accPropertyFormatted doesn't provide any idea that it contains a file name. Maybe defaultBehaviorFilename or defaultAccBehaviorFilename would work better (as this is, essentially, what is stored in this variable)?

kuzhelov

lets try to start with rename refactoring, and after that try to make the logic of ComponentDocTag's render() method a bit more clear and consistent with the previous behavior. Otherwise looks good to me :)

kuzhelov · 2018-09-10T09:07:40Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

@@ -1,27 +1,82 @@
 import * as React from 'react'
 import * as _ from 'lodash'
 import { Header, Message } from 'semantic-ui-react'
+const behaviorMenuItems = require('docs/src/behaviorMenu')


what I am worrying about here is the following: currently we are using pretty much generic name for this accessibility concern, like just behavior. Next day we are planning to add state management functionality to our components, and there is a chance that this state management aspects will be called as behaviors as well, but those won't have anything to share with accessibility. In that case generic behavior name would be confusing, as it would have two different meanings.

Won't suggest to change anything now, but this is something that we should keep in mind

FYI: @jurokapsiar

yes, we can have that discussion and change it later. it might be accessibility behavior for example. I will put it on the agenda for one of the next meetings

kuzhelov · 2018-09-10T09:09:02Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

+    }
+  }
+
+  renderDefaultBehavior(info) {


maybe it would be better to make the name of the arg more specific, info -> componentInfo

kuzhelov · 2018-09-10T09:22:54Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

-    const description = this.getTagDescription(tag, info) || (
-      <Message error content={errorMessage} compact={true} />
-    )
+    const accDescription = this.getTagDescription(tag, info)


this is a bit misleading - tag value could be anything, not just accessibility that we are interest in. Thus, we shouldn't name this variable as accDescription - it could be not the case. Also, not sure that we should change the logic of displaying errorMessage

I haven't introduced the method: getTagDescription(tag, info)
So wouldn't like to change origin code.
As I understand this method is responsible for get accessibility description from component itself. The acc description in component(s) still exists and is used.

I think we should change logic in displaying errorMessage because currently with our change there can appear following states:

both, acc description in component and default behavior acc description, exists

there is NO acc description in component but default behavior acc description exists

there is NO default behavior acc description but acc description in component exists

and last state when I think we should display error message is only when:

there is NO default behavior acc description and as well there is NO acc description in component

yes, sure - there is no need to change this method. Let me try to assist you with that, will suggest changes to make here (without touching existing functionality).

In terms of changing logic of displaying errorMessage - actually, I would suggest to display it anytime errorMessage is provided (not falsy). If there are any tweaks needed to be made for that, those should be done for the upper level of abstraction (for the place where ComponentDocTag is composed)

when you take look on original code:

render() { const { info, tag, title, errorMessage } = this.props const description = this.getTagDescription(tag, info) || ( <Message error content={errorMessage} compact={true} /> ) return ( <Header as="h2" style={headerStyle}> <Header.Content>{title}</Header.Content> <Header.Subheader>{description}</Header.Subheader> </Header> ) }

not sure what you ment by
" I would suggest to display it anytime errorMessage is provided (not falsy)"

because errorMessage is set to const "description" based of "TagDescription" availability...
In my opinion the errorMessage is not provided in the fact, but "description" and there can be anything...

kuzhelov · 2018-09-10T09:24:55Z

docs/src/components/ComponentDoc/ComponentDocTag.tsx

-    )
+    const accDescription = this.getTagDescription(tag, info)
+    const description =
+      accDescription || this.findDefaultBehaviorInfo(info) ? (


so, we are not using the result of findDefaultBehaviorInfo ? seems that we should call renderDefaultBehavior() here directly - otherwise we are calling the same method twice with no good reason for that

in this part of the code we just checking if there is any description or there should be error message. This part of structure I took from origin code.

We are using the result in this condition, because if there is no default behavior, then the method findDefaultBehaviorInfo() returns undefined...

About calling method twice. It is correct. Maybe we could save result of the method in some global variable and then when we will call it second time and global variable will exists, then we do not need to call it again. What do you think about?

kuzhelov · 2018-09-10T23:30:47Z

docs/src/components/DocsBehaviorRoot.tsx

    }

    const commentBox: React.CSSProperties = {
      padding: 5,
    }
+    const topRowStyle = {


it would be better to inline this style as well for now, for the sake of consistency

Milan Turon added 5 commits August 28, 2018 17:57

adding behavior description, and link which point to particular behav…

2c8e12c

…ior from component

Merge branch 'master' into feat/356643-acc-behaviour-description

1183810

Merge branch 'master' into feat/356643-acc-behaviour-description

b398ad8

moving description from component to behavior, modify and crate new d…

b5c7283

…escription

removing emtpy string

99d2c5c

kolaps33 requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners September 4, 2018 13:08

alinais reviewed Sep 4, 2018

View reviewed changes

mnajdova reviewed Sep 4, 2018

View reviewed changes

changes after PR

0c62278

miroslavstastny reviewed Sep 5, 2018

View reviewed changes

jurokapsiar reviewed Sep 5, 2018

View reviewed changes

miroslavstastny reviewed Sep 5, 2018

View reviewed changes

refactoring get default behavior

4ac477e

miroslavstastny approved these changes Sep 5, 2018

View reviewed changes

kuzhelov added the 🚀 ready for review label Sep 5, 2018

Milan Turon added 3 commits September 5, 2018 14:30

changes after PR, adding description on the top of page

8dd907b

Merge branch 'master' into feat/356643-acc-behaviour-description

d509d19

adding changelog

d1ba8fc

miroslavstastny reviewed Sep 5, 2018

View reviewed changes

Milan Turon and others added 2 commits September 5, 2018 15:06

moving tickets under documentation section

7395e81

Merge branch 'master' into feat/356643-acc-behaviour-description

b07c9ef

kuzhelov reviewed Sep 5, 2018

View reviewed changes

kuzhelov suggested changes Sep 5, 2018

View reviewed changes

kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Sep 5, 2018

Milan Turon added 3 commits September 6, 2018 09:39

changes in descriptions and desc. title

4f4e99f

Merge branch 'feat/356643-acc-behaviour-description' of https://githu…

96db925

…b.com/stardust-ui/react into feat/356643-acc-behaviour-description

renaming changes after PR

7dd5903

kuzhelov added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Sep 10, 2018

kuzhelov reviewed Sep 10, 2018

View reviewed changes

kuzhelov suggested changes Sep 10, 2018

View reviewed changes

Milan Turon added 2 commits September 10, 2018 11:36

renaming after PR

e8b3474

renaming 2 after PR

55dca0a

kuzhelov reviewed Sep 10, 2018

View reviewed changes

refactor logic of rendering behavior info

ea54e1d

kuzhelov approved these changes Sep 10, 2018

View reviewed changes

Merge branch 'master' into feat/356643-acc-behaviour-description

16c0e95

kuzhelov added ready for merge and removed 🚀 ready for review labels Sep 10, 2018

Milan Turon added 2 commits September 11, 2018 09:42

changing styles based on PR comment

d041bb1

Merge branch 'master' into feat/356643-acc-behaviour-description

5ae23d1

kolaps33 merged commit 9b4314b into master Sep 12, 2018

kuzhelov deleted the feat/356643-acc-behaviour-description branch November 19, 2018 15:59

feat(docs): adding accessibility description for behaviors #181

feat(docs): adding accessibility description for behaviors #181

Conversation

kolaps33 commented Sep 4, 2018 • edited

Adding description for behaviors

codecov bot commented Sep 4, 2018 • edited

Codecov Report

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

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

kuzhelov left a comment

Choose a reason for hiding this comment

kuzhelov commented Sep 10, 2018

Choose a reason for hiding this comment

kuzhelov Sep 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov 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

kolaps33 Sep 10, 2018 • edited

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

kolaps33 commented Sep 4, 2018 •

edited

codecov bot commented Sep 4, 2018 •

edited

kuzhelov Sep 10, 2018 •

edited

kolaps33 Sep 10, 2018 •

edited