8000 fix(menu): enable menu items with favorites to show star icon and set icon color when favorited by sg00dwin · Pull Request #7184 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

fix(menu): enable menu items with favorites to show star icon and set icon color when favorited#7184

Merged
mcoker merged 1 commit intopatternfly:mainfrom
sg00dwin:7180-menu-favorite-icon
Nov 25, 2024
Merged

fix(menu): enable menu items with favorites to show star icon and set icon color when favorited#7184
mcoker merged 1 commit intopatternfly:mainfrom
sg00dwin:7180-menu-favorite-icon

Conversation

@sg00dwin
Copy link
Contributor
@sg00dwin sg00dwin commented Oct 30, 2024

@patternfly-build
Copy link
Collaborator
patternfly-build commented Oct 30, 2024

@@ -2,19 +2,18 @@
{{setModifiers
menu-list-item--IsAriaDisabled='pf-m-aria-disabled'
menu-item-action--IsFavorite='pf-m-favorite'
Copy link
Member

Choose a reason for hiding this comment

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

Can this line be removed? It looks like menu-item-action--IsFavorite is used in the .hbs to create the icon, but we don't have .pf-m-favorite in menu.scss (Unless this is only used in react? The usage section says "Modifies the menu item action to handle the favorite icon.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you are correct, so I'll remove.

Though I did find pf-m-favorite referenced in the react component, but I don't see it utilizing any styling.

@sg00dwin sg00dwin force-pushed the 7180-menu-favorite-icon branch from b89f83f to 39735eb Compare November 7, 2024 20:24
Copy link
Member
@srambach srambach left a comment

Choose a reason for hiding this comment

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

Looks ⭐ !
Do you want to create a React followup to remove that class from the React component?

@sg00dwin
Copy link
Contributor Author

Looks ⭐ ! Do you want to create a React followup to remove that class from the React component?

New issue created patternfly/patternfly-react#11176

@mcoker mcoker self-requested a review November 18, 2024 15:41
Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM! I have a couple of small ideas as follow ups but they aren't blocking so I'll create a new issue to track those.

@mcoker mcoker merged commit c688b23 into patternfly:main Nov 25, 2024
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.1.0-prerelease.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug - Menu - favorite icon colors not showing

5 participants

0