patternfly
chore(docs): update menutoggle examples icon prop
Closes: #10841
This PR updates the <MenuToggle> components across all examples/demos to use the icon prop to pass icons where they are currently being passed as children.
This also updates one usage within the Table component: https://github.com/patternfly/patternfly-react/pull/11005/files#diff-5970fe3afb01cc875a418ab5399d91df662a78a5edc8f2c7c2e217f0315101ebR110-R114
Preview: https://patternfly-react-pr-11005.surge.sh
A11y report: https://patternfly-react-pr-11005-a11y.surge.sh
Just want to +1 removing <Icon> unless it's needed. Also if it's used, you'll need to pass isInline so that it inherits font size/color. You can see the icons here are not the correct color, but either removing <Icon> or using <Icon isInline> should fix it - https://patternfly-react-pr-11005.surge.sh/components/menus/menu-toggle#with-icons
This looks good to me. As an aside, I see aria-hidden="true" repeated in each instance. I suggest adding that as a default and optionally allowing an aria-attribute prop to override aria-hidden.
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 role="img", we would need the aria-hidden to hide it from AT yeah.
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 title prop is passed. I've also noticed we aren't always explicitly passing aria-hidden in React usage, so this may be a reason to just not do that if we already have that logic internal to our pf/react-icons that are created.
Looks like we also have a test suite for the createIcon function that tests for aria-hidden being true (maybe worth a followup to add a test to check that aria-hidden isn't set when title is passed). So a little more safe to just remove these explicit aria-hidden attributes on these icons, assuming we're not automatically passing a title elsewhere during icon creation.
@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 👍