chore(docs): update menutoggle examples icon prop#11005
chore(docs): update menutoggle examples icon prop#11005kmcfaul merged 3 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-11005.surge.sh A11y report: https://patternfly-react-pr-11005-a11y.surge.sh |
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
General comment. We do not need to wrap the icons in the <Icon> componet. The Icon componet should only be used when needed (e.g. sizing).
|
Just want to +1 removing |
|
This looks good to me. As an aside, I see |
|
The aria-hidden is on a lot but not every instance of the toggle icons, do we need it? Does it hurt to have or not have considering the toggles should all have aria-labels? @thatblindgeye |
|
Because the svg elements that are used have That said... the icons we typically import from the pf/react-icons package already have aria-hidden set to true by default; it's only ever not set when an icons Looks like we also have a test suite for the |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Awesome! I ran regression tests from core against patternfly-react main and chore/10841-menutoggle-icons (rebased against main before the run - no conflicts or anything). Your changes LGTM!
I think most of the errors are just
- noise from changes in dependencies (code editor and charts)
- dynamic content in some examples/demos that will likely always fail a visual comparison
- little artifacts in some of the examples with breadcrumbs on a mobile viewport - we get these in core for some reason, too
PDF report - backstop-menu-toggle-icon.pdf
Full report - https://drive.google.com/file/d/1YBJg1XNs-6bejS0aCj6eIUnasbZ5oLWF/view?usp=sharing
- unzip and open
backstop_data/html_report/index.html
Also spotted some other issues, but it goes beyond menu toggles needing to use the icon prop for icons, so created this issue - #11195
There was a problem hiding this comment.
LGTM! @thatblindgeye are we removing aria-hidden here?
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Also lgtm, I'll open a followup issue to review the aria-hidden across the repo if there isn't already one
|
@tlabaj oops just saw your tag above. I figure that might be better to do as a separate tech debt, removing aria-hidden here may touch a lot more files than is needed for this PR |
|
@thatblindgeye thanks again for the feedback here, sounds like follow-up work is warranted 👍 |
7 participants
Closes: #10841
This PR updates the
<MenuToggle>components across all examples/demos to use theiconprop to pass icons where they are currently being passed as children.This also updates one usage within the
Tablecomponent: https://github.com/patternfly/patternfly-react/pull/11005/files#diff-5970fe3afb01cc875a418ab5399d91df662a78a5edc8f2c7c2e217f0315101ebR110-R114