fix(JumpLinks): Fix aria issues #11950
Conversation
rebeccaalpert
force-pushed
the
issue-11395
branch
from
920d030 to
9838c07
Compare
|
Preview: https://patternfly-react-pr-11950.surge.sh A11y report: https://patternfly-react-pr-11950-a11y.surge.sh |
rebeccaalpert
force-pushed
the
issue-11395
branch
from
9838c07 to
b408d10
Compare
rebeccaalpert
requested review from
thatblindgeye,
a team and
wise-king-sullyman
and removed request for
a team
Remove aria-label from toggle if there is visible text. Add aria-labelled by if there is visible text. Added aria-labels to examples.
rebeccaalpert
force-pushed
the
issue-11395
branch
from
b408d10 to
e9ec49f
Compare
There was a problem hiding this comment.
Some comments below, also:
- for the "Expandable vertical with subsection" example, let's add an aria-label or aria-labelledby to the nested JumpLinksList in the example. Personally I'd love if this example used more unique text for each item for a more real-world like example, but I'd be fine just passing in an aria prop to JumpLinksList
- The "With label" example just needs to have their labels updated as they're both still "Jump to section"
| @@ -232,6 +243,11 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({ | |||
| return child; | |||
| }); | |||
|
|
|||
| const id = getUniqueId(); | |||
| const hasAriaLabelledBy = expandable || (label && alwaysShowLabel); | |||
| const computedAriaLabel = hasAriaLabelledBy ? null : ariaLabel; | |||
There was a problem hiding this comment.
This will be fine for now, but in the next breaking change we should tweak some of the logic for the aria in this component. Ideally we should allow both aria-labelledby and aria-label to be passed in (an element having an aria-label then referencing itself + some other element with an aria-labelledby).
Right now if we allowed that it'd cause redundancy by default whenever a label is passed in. #9802 can probably cover such updates especially since that will handle adding an aria-labelledby prop for customization as well.
packages/react-core/src/components/JumpLinks/examples/JumpLinksWithCenteredList.tsx
Outdated
Show resolved
Hide resolved
|
I think I addressed your comments @thatblindgeye but let me know if you want further changes! |
rebeccaalpert
force-pushed
the
issue-11395
branch
from
997dea1 to
ec9e566
Compare
There was a problem hiding this comment.
This is looking nice 😎 Only other thing is adding some tests. JumpLinks hasn't been revamped in its unit tests yet, but for sake of whomever does work on these remaining components it'd be good to help cover some of these new features. Typically what I've done is maybe add a comment in the test file at the end saying something like "Revamped tests begin here" or "Remove tests above here" or something, just some way to separate the old tests we'll most likely remove in a revamp and tests that should remain.
Just a few tests to add in this PR:
- labelId gets rendered properly on the correct element (when expandable is true and when it isnt)
- the random id is rendered when labelId is not passed
- the console warnings are thrown when expected and not thrown when not
the logic for the aria attributes I'll leave up to you if you want to include in tests. Those are mostly pre-existing things so I'd consider that part of the actual revamp of the JumpLinks unit tests.
|
I did half and half @thatblindgeye - added a couple of extra tests, but stuck mostly to the logic added here. |
What: Remove aria-label from toggle if there is visible text. Add aria-labelled by if there is visible text. Added aria-labels to examples. Made example labels unique.
Closes #11395
Additional issues: