Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(switch): add aria-labelledby to input #2468

Merged
merged 1 commit into from Aug 2, 2019
Merged

Conversation

@boaz0
Copy link
Member

boaz0 commented Jul 7, 2019

What:

closes #2435
closes #2600

//cc @jgiardino @tlabaj

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 7, 2019

PatternFly-React preview: https://patternfly-react-pr-2468.surge.sh

@tlabaj tlabaj requested review from jgiardino and tlabaj Jul 8, 2019
@tlabaj tlabaj self-assigned this Jul 8, 2019
@tlabaj tlabaj requested a review from kmcfaul Jul 8, 2019
@boaz0 boaz0 force-pushed the boaz0:close_2435 branch from cc3766e to 679707e Jul 25, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jul 25, 2019

resolved merge conflict. 😄

@boaz0 boaz0 force-pushed the boaz0:close_2435 branch 2 times, most recently from e9a972c to ef43b17 Jul 25, 2019
@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Jul 25, 2019

Thanks for contributing this, @boaz0!

When I modify the first example, and remove aria-label from the component, I see that aria-labelledby is added to the <input> as expected. I also see that id attributes are added to the <input> and also the <span> elements that provide the visible text labels, as expected. 🎉

There are just a couple of updates needed:

  • When aria-labelledby is added to the <input>, it should be set to the id value that is defined on the <span> that is visible during the on state.
    • Currently, the attribute is set to the id value assigned to the <input> as shown below:
      <input id="simple-switch" aria-label="" class="pf-c-switch__input" type="checkbox" aria-labelledby="simple-switch" checked="">
    • Instead, it should render like this:
      <input id="simple-switch" aria-label="" class="pf-c-switch__input" type="checkbox" aria-labelledby="simple-switch-on" checked="">
  • To align with our core examples, let's remove aria-label from the first example, so that the component is defined like this:
        <Switch
          id="simple-switch"
          label={isChecked ? 'Message when on' : 'Message when off'}
          isChecked={isChecked}
          onChange={this.handleChange}
        />
    
    And then when the html is rendered, this will include the aria-labelledby attribute like the example in core.
    • I think this would be sufficient for aligning with the core examples, but if we do decide to have complete alignment, then it's important to note that the react component examples showing the disabled switch would need to include both an on and off label (currently the disabled examples only include a string for the label that's visible).
@boaz0 boaz0 force-pushed the boaz0:close_2435 branch 2 times, most recently from e2cd456 to c197348 Jul 27, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jul 27, 2019

Thanks a lot @jgiardino on your input.

  • I updated this PR, can you please take a second look?
  • I opened #2600 to address your comment regarding the mismatch between core and react implementations.
@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Jul 30, 2019

@boaz0 will you be addressing issue #2600 here? A separate PR is good too.

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jul 30, 2019

separate PR 👍 unless you will merge it tomorrow

@boaz0 boaz0 force-pushed the boaz0:close_2435 branch from c197348 to 1ab4fe4 Jul 30, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jul 30, 2019

@tlabaj @jgiardino @redallen I updated the PR to address #2600 too.

Thanks.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:close_2435 branch from 1ab4fe4 to 009b968 Jul 30, 2019
Copy link
Contributor

redallen left a comment

LGTM

@kmcfaul
kmcfaul approved these changes Aug 1, 2019
@tlabaj
tlabaj approved these changes Aug 2, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 0f7cd6e into patternfly:master Aug 2, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 2, 2019

Your changes have been released in:

  • @patternfly/react-charts@4.7.3
  • @patternfly/react-core@3.81.0
  • @patternfly/react-docs@4.9.16
  • @patternfly/react-inline-edit-extension@2.9.62
  • demo-app-ts@2.14.6
  • @patternfly/react-table@2.16.2
  • @patternfly/react-topology@2.7.12
  • @patternfly/react-virtualized-extension@1.1.95

Thanks for your contribution! 🎉

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Aug 3, 2019

Thanks @tlabaj @kmcfaul @redallen ! 😄

@boaz0 boaz0 deleted the boaz0:close_2435 branch Aug 3, 2019
@SpyTec

This comment has been minimized.

Copy link
Contributor

SpyTec commented Sep 2, 2019

This introduced a breaking change in a minor version (from v3.80.4 to v3.81.0). Prior to this the label prop was be shown both when on and off. Not it only shows when on and not when off.

We have to explicitly add both label and labelOff even when we don't want to change the text. Was this intended?

Opened an issue #2814

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.