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

chore(docs): update menutoggle examples icon prop #11005

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

evwilkin
Copy link
Member

@evwilkin evwilkin commented Sep 15, 2024

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

@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 15, 2024

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@mcoker
Copy link
Contributor

mcoker commented Sep 27, 2024

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

@mattnolting
Copy link
Contributor

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.

@evwilkin evwilkin requested a review from tlabaj October 30, 2024 14:12
@kmcfaul
Copy link
Contributor

kmcfaul commented Nov 8, 2024

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

@thatblindgeye
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MenuToggle - icons should be passed via icon prop
7 participants