feat(button): add favorite variant #11853

Mash707 · 2025-05-28T21:53:53Z

patternfly-build · 2025-05-28T21:57:54Z

Preview: https://patternfly-react-pr-11853.surge.sh

A11y report: https://patternfly-react-pr-11853-a11y.surge.sh

thatblindgeye

Just some initial comments below, the animation works nicely so far ⭐

We should also add a console warning if isFavorite && !ariaLabel stating that each favorite button must have a unique aria-label that indicates the current favorited state

packages/react-core/src/components/Button/Button.tsx

packages/react-core/src/components/Button/examples/Button.md

packages/react-core/src/components/Button/examples/ButtonFavorite.tsx

thatblindgeye · 2025-06-03T18:14:05Z

@srambach @andrew-ronaldson just FYI, the no-padding variant is visually broken. If you go to the favorite button example from this PR and add hasNoPadding to the button component in the example code, you should notice that there's no color change when favorited.

srambach · 2025-06-03T20:18:34Z

@srambach @andrew-ronaldson just FYI, the no-padding variant is visually broken. If you go to the favorite button example from this PR and add hasNoPadding to the button component in the example code, you should notice that there's no color change when favorited.

Hmm yes, looks like we're setting the icon color for a plain no-padding button and that's overriding the favorited color. https://github.com/patternfly/patternfly/blob/d375e1dc0f924f11149fe105875caedbcefd1705/src/patternfly/components/Button/button.scss#L617

andrew-ronaldson · 2025-06-04T00:42:10Z

We also have the unfilled to filled icon in core
https://core-staging.patternfly.org/components/button#favorite

thatblindgeye

Lengthy comment below.

We should also add some unit tests for the new props/classes being applied.

thatblindgeye · 2025-06-04T12:24:55Z

packages/react-core/src/components/Button/examples/ButtonFavorite.tsx

+      isFavorite
+      isFavorited={isFavorited}
+      onClick={toggleFavorite}
+      icon={<StarIcon />}


In addition to Andrew's comment above about using the outlined star icon when the button isn't favorited, we should apply the icons internal to the Button component when it's a favorite button since we also need to wrap these star icons in additional class wrappers and both icons should be rendered in the DOM at the same time (rather than dynamically rendering the correct icon, as these additional class wrappers will determine which one is hidden or not).

Here's the Core markup for the icons:

vs the markup in the PR preview:

Though @andrew-ronaldson would we want to allow a consumer to customize the favorite icon(s) at all? If we always have both icons rendered we'd most likely need to add some other props (favoriteIcon and favoritedIcon or something) since we need to place the correct icons in the correct icon wrapper. Dynamically rendering the correct star icon might be a little cleaner, since then you only need to pass in the existing icon prop, and the animation still seems to work, but it makes some of the Core styling unused in React/creates a mismatch in markup between Core and React.

Figure I'd ask this now because depending on the answer (and the approach if the answer is "yes"), might be less of a headache to deal with it now than try to finagle something without causing a breaking change to the markup later.

I don't think we need to allow for their own custom favorite icon. Side note, Would it be better if it was one icon with a stroke and a fill that gets set by a class? the hamburger icon is unique and we play with paths so thought i'd ask.

If we go into it with the stance that we don't want consumers to be able to customize the "favorites" icon, I think the current implementation (2 icons that need their own distinct wrappers) isn't too bad. 1 icon that just gets a class applied to handle the filling of the icon would be less to deal with, but I don't think it's absolutely needed, especially since - even though it would probably be a quick Core update - it's still time being taken away from any remaining animations work we need to get done.

thatblindgeye · 2025-06-04T12:41:22Z

packages/react-core/src/components/Button/examples/Button.md

+### Favorite
+
+You can pass both the `isFavorite` and `variant="plain"` properties into the `<Button>` to create a favorite button. Passing the `isFavorited` property will determine the current favorited state and update styling accordingly.


@edonehoo can you give this a quick review?

thatblindgeye

Just a couple comments that need design input below cc @andrew-ronaldson @kaylachumley , but this is continuing to shape up awesomely

thatblindgeye · 2025-06-06T19:45:45Z

packages/react-core/src/components/Button/Button.tsx

+        isFavorite && variant === ButtonVariant.plain && styles.modifiers.favorite,
+        isFavorite && isFavorited && variant === ButtonVariant.plain && styles.modifiers.favorited,


This would need design input, but rather than needing the consumer to explicitly pass the plain variant in, we could force the plain button styling when isFavorite is true.

Per conversation from an animation sync today, we want to not restrict the variant for these buttons. Our design guidelines will recommend when to use what variants for these. So we can remove the && variant === ButtonVariant.plain from these 2 conditionals (but keep the example so that we're passing variant="plain")

thatblindgeye · 2025-06-06T19:46:37Z

packages/react-core/src/components/Button/Button.tsx

+  };
+
+  const _icon = renderIcon();
  const _children = children && <span className={css('pf-v6-c-button__text')}>{children}</span>;


Also design input needed, but if intend to not allow text content for a favorite button, we could add a && !isFavorite check here.

Disregard this comment, we don't want to restrict the rendering of children for these buttons

srambach · 2025-06-11T14:55:18Z

Looks good to me! ⭐

rebeccaalpert

Looks good to me. Animation is very cute!

thatblindgeye

In addition to the below comments, let's add another test that checks that the icon prop is overridden when isFavorite is passed in.

thatblindgeye · 2025-06-11T19:03:44Z

packages/react-core/src/components/Button/__tests__/Button.test.tsx

+test('Renders favorite icon with class pf-m-favorite when isFavorited = false', () => {
+  render(<Button isFavorite />);
+  expect(screen.getByRole('button')).toHaveClass('pf-m-favorite');
+});


For this test, let's use the imported styles object rather than hardcoding the class names to expect. So we can replace pf-m-favorite with styles.modifiers.favorite (or whatever the modifier property is, I assume it'd be favorite though) both in the test name and the toHaveClass call

Additionally, let's update the test name to reflect more what that test is expecting: "Renders with class [classname here] when isFavorite is true".

And then last thing is we should include a test that checks this class isn't applied by default/when isFavorite is false.

thatblindgeye · 2025-06-11T19:04:28Z

packages/react-core/src/components/Button/__tests__/Button.test.tsx

+test('Renders favorite icon with class pf-m-favorite when isFavorited = true', () => {
+  render(<Button isFavorite isFavorited />);
+  expect(screen.getByRole('button')).toHaveClass('pf-m-favorited');
+});


Same updates here as the above test: using the styles object, updating the test name, and including a test that check the pf-m-favorited class isn't applied by default

andrew-ronaldson

you're a ⭐

thatblindgeye

Awesome work! 🤩 You could say this is my pf-m-favorite

patternfly-build · 2025-06-13T13:52:32Z

Your changes have been released in:

@patternfly/react-code-editor@6.3.0-prerelease.19
@patternfly/react-core@6.3.0-prerelease.19
@patternfly/react-docs@7.3.0-prerelease.25
@patternfly/react-drag-drop@6.3.0-prerelease.19
demo-app-ts@6.0.0-prerelease.122
@patternfly/react-table@6.3.0-prerelease.19
@patternfly/react-templates@6.3.0-prerelease.19

Thanks for your contribution! 🎉

thatblindgeye marked this pull request as draft May 29, 2025 11:00

thatblindgeye reviewed May 29, 2025

View reviewed changes

Mash707 requested a review from thatblindgeye June 3, 2025 14:10

srambach mentioned this pull request Jun 3, 2025

Bug - Plain no-padding button is missing favorited coloring patternfly/patternfly#7559

Closed

thatblindgeye marked this pull request as ready for review June 4, 2025 12:13

thatblindgeye requested review from a team, andrew-ronaldson, rebeccaalpert and srambach and removed request for a team June 4, 2025 12:13

thatblindgeye reviewed Jun 4, 2025

View reviewed changes

thatblindgeye requested a review from nicolethoen June 5, 2025 17:13

Mash707 force-pushed the button-add-favorite-variant branch from 27c1ba7 to 46fddb2 Compare June 6, 2025 17:45

thatblindgeye reviewed Jun 6, 2025

View reviewed changes

srambach approved these changes Jun 11, 2025

View reviewed changes

rebeccaalpert approved these changes Jun 11, 2025

View reviewed changes

thatblindgeye requested changes Jun 11, 2025

View reviewed changes

andrew-ronaldson approved these changes Jun 12, 2025

View reviewed changes

Mash707 added 7 commits June 13, 2025 00:41

feat(button): add favorite variant

93d6a04

fix icon import

5957909

improved docs and made aria label dynamic

178fad4

added the star icon

e7a917b

added console warning for a11y

c5a67d7

updated icon render logic

7141428

star icon updated

f902f48

Mash707 added 3 commits June 13, 2025 00:41

removed the plain variant condition

2867b1d

added unit tests

64123db

added unit tests

d3f6e65

Mash707 force-pushed the button-add-favorite-variant branch from af39a79 to d3f6e65 Compare June 12, 2025 19:12

thatblindgeye approved these changes Jun 13, 2025

View reviewed changes

thatblindgeye merged commit 84c5af6 into patternfly:main Jun 13, 2025
16 of 19 checks passed

Mash707 deleted the button-add-favorite-variant branch June 26, 2025 12:12

jenny-s51 mentioned this pull request Aug 6, 2025

Button - support isFavorite and isFavorited on Button for animation styling patternfly/pf-codemods#853

Closed

		### Favorite

		You can pass both the `isFavorite` and `variant="plain"` properties into the `<Button>` to create a favorite button. Passing the `isFavorited` property will determine the current favorited state and update styling accordingly.

		isFavorite && variant === ButtonVariant.plain && styles.modifiers.favorite,
		isFavorite && isFavorited && variant === ButtonVariant.plain && styles.modifiers.favorited,

feat(button): add favorite variant #11853

feat(button): add favorite variant #11853

Uh oh!

Conversation

Mash707 commented May 28, 2025

Uh oh!

patternfly-build commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thatblindgeye commented Jun 3, 2025

Uh oh!

srambach commented Jun 3, 2025

Uh oh!

andrew-ronaldson commented Jun 4, 2025

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srambach commented Jun 11, 2025

Uh oh!

rebeccaalpert left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-ronaldson left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patternfly-build commented Jun 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

patternfly-build commented May 28, 2025 •

edited

Loading