Add aria-describedby attribute to date input #266

allanpope · 2017-01-26T03:54:03Z

When using a screen reader you are able to type in a date but you don't know about any restrictions the date component might have.

This pull request adds the ability to inform screen reader users about the constraints of the component they are using via the aria-describedby attribute.

Any text passed into the new screenReaderInputMessage prop will be read out when a screen reader user interacts with the input.

Closes #261

coveralls · 2017-01-26T04:34:35Z

Coverage remained the same at 84.857% when pulling 93eaca7 on allanpope:master into 67c0fb5 on airbnb:master.

coveralls · 2017-01-26T04:41:14Z

Coverage remained the same at 84.857% when pulling 93eaca7 on allanpope:master into 67c0fb5 on airbnb:master.

majapw · 2017-01-27T01:28:42Z

@backwardok or @wyattdanger would you mind taking a look at this PR as our resident a11y experts?

wyattdanger

Thanks, @allanpope! Overall implementation looks good, but I have a few requests

wyattdanger · 2017-01-29T23:44:07Z

test/components/DateInput_spec.jsx

@@ -58,6 +58,46 @@ describe('DateInput', () => {
      });
    });

+    describe('screen reader message', () => {


Looks like we'll need similar specs for the other components touched in the PR

Just had another look at the tests. I'm unsure what other components I could test?

All the tests files use enzymes shallow rendering, so I can only test that level of the component. The changes I've added to them are pretty simple. All they do is take a screenReaderMessage prop and pass it to a child component. This prop is passed all the way down until it gets to the DateInput component, which the above tests cover.

Happy to add more tests but need some further instruction, thanks.

wyattdanger · 2017-01-29T23:54:29Z

README.md

@@ -194,6 +194,11 @@ If the `required` prop is set to true, the input will have to be filled before t
  required: PropTypes.bool,
 ```

+The `screenReaderInputMessage` prop allows you to write a message for screen readers. When the prop is added a `aria-describedby` attribute will be added to the input box and the value will be read by screen readers. Take advantage of this to inform users about constraints your component has, such as the date format, minimum nights and blocked out dates etc.


Would like to see a few grammatical changes here, but overall looks like reasonable documentation! Thanks for including this. Maybe something a bit more in this direction?

The screenReaderInputMessage prop accepts a contextual message for screen readers. When an input is focused, the screenReaderInputMessage prop value is read. This can inform users about constraints, such as the date format, minimum nights, blocked out dates, etc.

No problem. Thanks that reads a lot better.

wyattdanger · 2017-01-29T23:55:50Z

src/components/DateInput.jsx

@@ -128,8 +131,15 @@ export default class DateInput extends React.Component {
          disabled={disabled}
          readOnly={this.isTouchDevice}
          required={required}
+          aria-describedby={screenReaderMessage && `DateInput__screen-reader-message-${id}`}


Would prefer to see this string interpolated only once per render, perhaps at the top of the render method

Ah good pickup, I didn't consider that.

coveralls · 2017-01-30T09:24:04Z

Coverage increased (+0.02%) to 85.16% when pulling 53e29c7 on allanpope:master into 0bd7072 on airbnb:master.

ljharb · 2017-01-30T16:18:14Z

src/components/SingleDatePicker.jsx

@@ -333,6 +333,7 @@ export default class SingleDatePicker extends React.Component {
      phrases,
      withPortal,
      withFullScreenPortal,
+      screenReaderInputMessage,


Please add this prop to propTypes/defaultProps

Completely missed this one, have added it for this and DateRangePicker.

ljharb · 2017-01-30T16:18:39Z

stories/DateRangePicker.js

+  ))
+  .addWithInfo('with screen reader message', () => (
+    <DateRangePickerWrapper
+      screenReaderInputMessage='Here you could inform screen reader users of the date format, minimum nights, blocked out dates etc'


Please use an Oxford comma after "dates" :-)

ljharb · 2017-01-30T16:18:45Z

stories/SingleDatePicker.js

+  ))
+  .addWithInfo('with screen reader message', () => (
+    <SingleDatePickerWrapper
+      screenReaderInputMessage='Here you could inform screen reader users of the date format, minimum nights, blocked out dates etc'


ljharb · 2017-01-30T16:21:10Z

test/components/DateInput_spec.jsx

+      const id = 'date';
+      const screenReaderMessage = 'My screen reader message';
+      const screenReaderMessageName = `DateInput__screen-reader-message-${id}`;
+      const screenReaderMessageId = `#${screenReaderMessageName}`;


Technically the ID is line 75, and this is the "selector" - could we improve the var names a bit?

Yeah your correct, I've updated the names of these variables.

I also moved the creation of the wrappers into a before loop since they were all the same, which reduces the duplication and makes it easier to read.

That's fine - although duplication in tests often helps.

coveralls · 2017-01-31T00:20:04Z

Coverage increased (+0.02%) to 85.16% when pulling 73b6984 on allanpope:master into 0bd7072 on airbnb:master.

@wyattdanger

deferring to @wyattdanger

wyattdanger · 2017-01-31T19:57:09Z

Great! Please rebase, squash, write a solid commit message, and then we'll be good to ship. Thank you Allan

allanpope · 2017-02-01T00:05:55Z

@wyattdanger @ljharb Thanks for taking a look at it.

I've just rebased, squashed & committed.

coveralls · 2017-02-01T00:14:54Z

Coverage increased (+0.02%) to 85.16% when pulling 8230328 on allanpope:master into 0bd7072 on airbnb:master.

…f the date component. New prop has been added called screenReaderInputMessage. It's value will be read outloud by a screen reader when the input is focused on. * Add ability to pass down a screen reader message to the date input, taking advantage of the aria-describedby attribute. * Add an example of how to attach a screen read message to an date input. * Append component id to screen reader message to make sure its unique, incase multiple date pickers are on a page. * Add tests to make sure screen reader text is displays correctly. * Add documentation about screenReaderInputMessage prop. * Make grammatical changes to screenReaderInputMessage docs * Move string interpolation out into a const so that it only happens once per a render. * Pull shared variables up a level to reduce amount of times string interpolation needs to happen * Add screen reader input message prop to propTypes & defaultPropTypes * Pull out wrapper creation into before loop since they are all the same.

coveralls · 2017-02-01T21:24:56Z

Coverage increased (+0.02%) to 86.926% when pulling 31307c8 on allanpope:master into a7eeda2 on airbnb:master.

coveralls · 2017-02-01T21:42:55Z

Coverage increased (+0.02%) to 86.926% when pulling ab9c5d9 on allanpope:master into a7eeda2 on airbnb:master.

Confirmed offline that this is good to go!

wyattdanger previously requested changes Jan 29, 2017

View reviewed changes

ljharb previously requested changes Jan 30, 2017

View reviewed changes

allanpope force-pushed the master branch from 73b6984 to 8230328 Compare February 1, 2017 00:00

allanpope force-pushed the master branch from 8230328 to 31307c8 Compare February 1, 2017 21:07

Merge branch 'master' into master

ab9c5d9

majapw approved these changes Feb 1, 2017

View reviewed changes

majapw merged commit 4f9cfbc into react-dates:master Feb 1, 2017

majapw added the semver-minor: new stuff Any feature or API addition. label Feb 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add aria-describedby attribute to date input #266

Add aria-describedby attribute to date input #266

allanpope commented Jan 26, 2017

coveralls commented Jan 26, 2017

coveralls commented Jan 26, 2017

majapw commented Jan 27, 2017

wyattdanger left a comment

wyattdanger Jan 29, 2017

allanpope Jan 30, 2017

wyattdanger Jan 29, 2017

allanpope Jan 30, 2017

wyattdanger Jan 29, 2017

allanpope Jan 30, 2017 •

edited

Loading

coveralls commented Jan 30, 2017

ljharb Jan 30, 2017

allanpope Jan 30, 2017

ljharb Jan 30, 2017

allanpope Jan 30, 2017

ljharb Jan 30, 2017

ljharb Jan 30, 2017

allanpope Jan 30, 2017

ljharb Jan 31, 2017

coveralls commented Jan 31, 2017

wyattdanger commented Jan 31, 2017

allanpope commented Feb 1, 2017

coveralls commented Feb 1, 2017

coveralls commented Feb 1, 2017

coveralls commented Feb 1, 2017

Add aria-describedby attribute to date input #266

Add aria-describedby attribute to date input #266

Conversation

allanpope commented Jan 26, 2017

coveralls commented Jan 26, 2017

coveralls commented Jan 26, 2017

majapw commented Jan 27, 2017

wyattdanger 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

allanpope Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jan 30, 2017

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

coveralls commented Jan 31, 2017

wyattdanger commented Jan 31, 2017

allanpope commented Feb 1, 2017

coveralls commented Feb 1, 2017

coveralls commented Feb 1, 2017

coveralls commented Feb 1, 2017

allanpope Jan 30, 2017 •

edited

Loading