Skip to content

DetailsList: select all button should not have aria-describedby if no label is present#6682

Merged
natalieethell merged 5 commits into
microsoft:masterfrom
natalieethell:detailsListSelectAll
Oct 12, 2018
Merged

DetailsList: select all button should not have aria-describedby if no label is present#6682
natalieethell merged 5 commits into
microsoft:masterfrom
natalieethell:detailsListSelectAll

Conversation

@natalieethell

@natalieethell natalieethell commented Oct 12, 2018

Copy link
Copy Markdown
Contributor

Pull request checklist

Description of changes

The aria-describedby attribute of the select all button in DetailsList should only be set to {this._id}-checkTooltip if the label with that id exists. I added the same conditional used for adding the label with id {this._id}-checkTooltip to adding aria-describedby with id {this._id}-checkTooltip.

These changes also add aria labels to select all checkboxes in all DetailsList examples.

Focus areas to test

Added two tests in DetailsHeader.test.tsx to verify the following two scenarios:

  1. Ensure that aria-describedby does not exist on the select all button when an ariaLabelForSelectAllCheckbox prop is not passed in.
  2. Ensure that aria-describedby exists on the select all button when an ariaLabelForSelectAllCheckbox prop is passed in and an onRenderColumnHeaderTooltip is not passed in.
Microsoft Reviewers: Open in CodeFlow

@KevinTCoughlin KevinTCoughlin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

.first()
.getDOMNode()
.getAttribute('aria-describedby')
).toBeNull();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is totally fine, but you can also do:

    expect(
      component
        .find(`#${selectAllCheckBoxAriaLabelledBy}`)
        .first()
        .getDOMNode()
        .hasAttribute('aria-describedby')
    ).toBe(false);

@natalieethell natalieethell mentioned this pull request Oct 12, 2018
8 tasks
@natalieethell
natalieethell merged commit 84f3757 into microsoft:master Oct 12, 2018
@msft-github-bot

Copy link
Copy Markdown
Contributor

🎉office-ui-fabric-react@v6.84.1 has been released which incorporates this pull request.:tada:

Handy Links:

@natalieethell

Copy link
Copy Markdown
Contributor Author

Fixes #7 and #8 in #6646, too.

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.

DetailList - select all button, adding aria-describedby attribute even if no label is provided

4 participants