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

fix(radio): group radio inputs in docs (axe-core) #2684

Merged
merged 1 commit into from Oct 3, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Aug 12, 2019

What:

closes #2568

Radio inputs that have the same name should be grouped. One way to group them is inside a div with role="group" and aria-label to describe this group.

//cc @christiemolloy

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 12, 2019

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

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Aug 22, 2019

Thanks for working on this, @boaz0! First of all, your solution is valid in terms of accessibility. However, I question if we should be fixing this issue at all.

If I use the Radio component in the context of a form, I would expect to provide a visible label that names the group of radio buttons.

There is an issue related to this in core that's still open and provides a suggestion on how to handle that: patternfly/patternfly-next#953

That issue is in the context of the Form component, which would consume the Radio component. If we want to handle how radio buttons are grouped, I think that would be addressed in the Form component.

If we were to update the Radio component with this PR, how would that update affect the ability to define the group and group label in the context of the Form component?

If we rely on this grouping and naming to happen in the Form component, is it ok to have this component fail the a11y audit?

My concern with trying to make things accessible out of context is that the consumer might not realize that the additional markup was only included because the component was out of context. My suggestion would be to not address a11y failures in situations like this, and instead refer the consumer to a component or demo that illustrates how to use the component in an accessible way.

I'm curious what others think about this.

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Aug 23, 2019

@jgiardino 👍

How about aligning the example with what we have in the pf4 core here - it will fix the a11y message till patternfly/patternfly-next#953 will be solved.

What do you think?

Thanks.

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Aug 27, 2019

@boaz0
That link to codepen doesn't generate an example from PF. Do you have another link you can share?

@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Aug 27, 2019

I think that there's a valid use case to use Radio buttons outside of forms. Otherwise we shouldn't be providing these examples of them outside forms. We can include the details that Jenn mentioned on how to make them accessible in the context of a form in the Form's documentation page.

@jgiardino Let's talk about what developers should know about forms and accessibility and write some docs!

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Aug 28, 2019

My apologies on being so dense on this one. I needed the CSS army to explain to me that our example of the Radio component passes the aXe audit because we're using different values for the name attribute. 🤦‍♀ And having more than one radio button with the same name but not in a group will fail, which is what's happening with our react examples.

@boaz0 If you were referring to this example in core, then yes, I think aligning with that example makes sense.

@boaz0 boaz0 force-pushed the boaz0:closes_2568 branch from d68bdb2 to 81f7aa2 Aug 29, 2019
@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Aug 29, 2019

👍 Works for me. Thanks for updating this, @boaz0!!

Copy link
Contributor

redallen left a comment

On second look, the buttons are broken on the preview link and both are checked at the same time.

Copy link
Contributor

jessiehuff left a comment

@boaz0 I was just about to say the same thing as Zack above. I noticed the radio functionality seems to be broken. :(

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Sep 4, 2019

I'm not sure if this is still the goal or not, but my understanding with creating examples is that they should be similar between core and react. The example from core also doesn't wrap the radio buttons in the same group (i.e. creating the behavior you described).

@jessiehuff @redallen
If there is an issue with the example created for this PR, I would suggest raising that question with both core and react contributors to figure out what the example should be before asking @boaz to continue making changes.

I also question if that's something that should block this PR or not. I thought the scope of this PR was to address the accessibility issue found by aXe, and since the behavior between core and react examples seem to match, I think this PR could move forward. 🤷‍♀

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Sep 4, 2019

Currently I can't click out of the radio checkboxes, are you getting the same thing?
Screen Shot 2019-09-04 at 11 22 33 AM

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Sep 4, 2019

Here is the example from html. The ability to select a radio button in a group is native to html, and not functionality that's provided by javascript.

cc'ing @mcoker on this to weigh in on whether we should be wrapping the Radio component with additional things that are not part of the component in our component examples (e.g. like a <fieldset>), or if that type of example belongs somewhere else, like a demo?

image

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 4, 2019

@jgiardino this PR looks good to me. My expectation is that the examples should match, too, whenever possible.

That said, for this example, I think the examples should just ideally just represent the component, and not include anything that isn't necessary to demonstrate the component's functionality. Here I think labeling the radio button examples as checked/unchecked would suffice to serve as an example for the radio component, then we illustrate how radio buttons should be named, grouped, etc in the context of a form in a demo.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 4, 2019

We chatted about it as a team and decided to keep any additional form elements needed to create a group of radio buttons in a form out of the component examples, and since it's a big confusing now that you can't toggle between radio buttons (like you would in a form), just to update the examples so we only have a single radio in the example, instead of a group. I created an issue to do that in the core workspace here - patternfly/patternfly-next#2221. Can we do that for the examples in the react workspace, too?

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 5, 2019

@mcoker yes - I will update according to patternfly/patternfly-next#2221
no problem. 😸

@boaz0 boaz0 force-pushed the boaz0:closes_2568 branch from 81f7aa2 to d6d14bc Sep 8, 2019
@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Sep 19, 2019

Thanks for making these updates! I only see a couple of updates needed for the example Label wraps input radio:

  • The element that has the class pf-c-radio should be a <label> and not a <div> in this case.
  • The for attribute that is on the <span> should be moved to the label.pf-c-radio element.
@boaz0 boaz0 force-pushed the boaz0:closes_2568 branch from 0f77579 to ea9a24e Sep 24, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 24, 2019

@jgiardino thank you for the review.
A question: is it possible that both the radio is reversed and the label wraps input?

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 24, 2019

is it possible that both the radio is reversed and the label wraps input?

@boaz0 yep!

@boaz0 boaz0 force-pushed the boaz0:closes_2568 branch from ea9a24e to e4fe3b9 Sep 25, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 25, 2019

@jgiardino @mcoker done - I added isReversed and isWrapped props and discarded the variant prop idea.

Let me know what you think.

I have a question though when I am in "label wraps input" making the window slower doesn't put the label in one row and the radio button in the second row. Am I missing something?

Thanks.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 25, 2019

I have a question though when I am in "label wraps input" making the window slower doesn't put the label in one row and the radio button in the second row. Am I missing something?

Ah, the difference between the normal and "label wraps input" variations doesn't have anything to do with content wrapping onto multiple lines. It shows how to have the <label> html element wrap the <input type="radio"> instead of being adjacent to it. The normal markup is like:

div.pf-c-radio
   input.pf-c-radio__input
   label.pf-c-radio__label

And when the label wraps, it's:

label.pf-c-radio
    input.pf-c-radio__input
    span.pf-c-radio__label
@boaz0 boaz0 force-pushed the boaz0:closes_2568 branch from e4fe3b9 to 3994f44 Sep 27, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 27, 2019

@tlabaj thank you so much for the review - that was very helpful.
@mcoker thanks a lot for the explanation. I also appreciate your time testing and reviewing again and again.

Copy link
Contributor

mcoker left a comment

lgtm! thanks @boaz0!!!

Copy link
Contributor

jessiehuff left a comment

LGTM - Great job! :)

Copy link
Contributor

tlabaj left a comment

LGTM. @boaz0 you Just need to resolve conflicts.

@boaz0 boaz0 dismissed stale reviews from tlabaj and jessiehuff via cfe5add Oct 2, 2019
@boaz0 boaz0 force-pushed the boaz0:closes_2568 branch from 3994f44 to cfe5add Oct 2, 2019
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:closes_2568 branch from cfe5add to 31d8203 Oct 2, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 2, 2019

Codecov Report

Merging #2684 into master will increase coverage by <.01%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2684      +/-   ##
==========================================
+ Coverage   68.97%   68.97%   +<.01%     
==========================================
  Files         857      857              
  Lines       23445    23452       +7     
  Branches     1847     1850       +3     
==========================================
+ Hits        16171    16177       +6     
  Misses       6357     6357              
- Partials      917      918       +1
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.22% <ø> (ø) ⬆️
#patternfly4 68.01% <84.61%> (+0.01%) ⬆️
Impacted Files Coverage Δ
...ernfly-4/react-core/src/components/Radio/Radio.tsx 88.88% <84.61%> (-0.77%) ⬇️

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 26d7a7f...31d8203. Read the comment docs.

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Oct 2, 2019

@tlabaj conflict was fixed.

@tlabaj
tlabaj approved these changes Oct 2, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Contributor

jessiehuff left a comment

LGTM!

@jessiehuff jessiehuff merged commit de22a80 into patternfly:master Oct 3, 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 Oct 3, 2019

Your changes have been released in:

  • @patternfly/react-core@3.112.7
  • @patternfly/react-docs@4.14.7
  • @patternfly/react-inline-edit-extension@2.11.75
  • demo-app-ts@3.6.14
  • @patternfly/react-table@2.22.24
  • @patternfly/react-topology@2.8.69
  • @patternfly/react-virtualized-extension@1.2.59

Thanks for your contribution! 🎉

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Oct 3, 2019

Thank you all!! 🎉

@boaz0 boaz0 deleted the boaz0:closes_2568 branch Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.