Skip to content

Conversation

dleroux
Copy link
Contributor

@dleroux dleroux commented Jul 12, 2019

WHY are these changes introduced?

Fixes #1589

WHAT is this pull request doing?

  1. add ariaDescribedBy prop to Checkbox and RadioButton
  2. remove aria-describedby from the ChoiceList fieldset
  3. updated the ChoiceDescriptor with a describedByError key to indicate whether to link the Error to the input or not.
  4. added some tests

How to 🎩

Using the playground, ensure the radio buttons aria-describedby attribute matches the ID of the error field.

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Page, ChoiceList} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="Playground">
        <ChoiceListExample />
      </Page>
    );
  }
}

class ChoiceListExample extends React.Component {
  state = {
    selected: ['hidden'],
  };

  render() {
    const {selected} = this.state;
    return (
      <ChoiceList
        title="Company name"
        choices={[
          {label: 'Hidden', value: 'hidden', describedByError: true},
          {label: 'Optional', value: 'optional'},
          {label: 'Required', value: 'required'},
        ]}
        selected={selected}
        onChange={this.handleChange}
        error="Company name cannot be hidden at this time"
      />
    );
  }

  handleChange = (value) => {
    this.setState({selected: value});
  };
}

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-1824 July 12, 2019 15:03 Inactive
@dleroux dleroux force-pushed the choice-list-aria-described-by branch from e0da909 to b52f915 Compare July 12, 2019 15:12
@BPScott BPScott temporarily deployed to polaris-react-pr-1824 July 12, 2019 15:13 Inactive
@dleroux dleroux force-pushed the choice-list-aria-described-by branch from b52f915 to 737607f Compare July 12, 2019 15:20
@BPScott BPScott temporarily deployed to polaris-react-pr-1824 July 12, 2019 15:20 Inactive
const describedBy: string[] = [];
if (error) {
describedBy.push(`${id}Error`);
if (error && typeof error !== 'boolean') {
Copy link
Contributor Author

@dleroux dleroux Jul 12, 2019

Choose a reason for hiding this comment

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

If the type is boolean we don't want an area-describedby attribute

/** Additional text to aide in use */
helpText?: React.ReactNode;
/** Indicates that the choice is aria-describedBy the error message*/
describedByError?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I went with describedByError: boolean as oppose to describedBy:string is because ChoiceList is the one generating the error and the id. So I thought to add describedByError to the descriptor was more clear. The choice has 1 error and this choice is described by that error.

@dleroux dleroux force-pushed the choice-list-aria-described-by branch from 737607f to a088685 Compare July 12, 2019 15:31
@BPScott BPScott temporarily deployed to polaris-react-pr-1824 July 12, 2019 15:31 Inactive
@dleroux dleroux changed the title [WIP] [Choicelist] Connecting the ChoiceList error the input with errors via describedByProp [ChoiceList] Connecting the ChoiceList error the input with errors via describedByProp Jul 12, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-1824 July 12, 2019 16:41 Inactive
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

The API update makes sense to me, and the aria-describedby is now being set on the input as recommended 🚢

@dleroux dleroux force-pushed the choice-list-aria-described-by branch from a520f9b to 17ede77 Compare July 17, 2019 12:36
@BPScott BPScott requested a deployment to polaris-react-pr-1824 July 17, 2019 12:36 Abandoned
@dleroux dleroux merged commit 8c32faf into master Jul 17, 2019
@alex-page alex-page temporarily deployed to production July 31, 2019 14:28 Inactive
@dleroux dleroux deleted the choice-list-aria-described-by branch August 14, 2019 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[a11y] [Choice list] Error message not programmatically clear

4 participants