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.

@edonehoo can you give this a quick review?

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

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

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.

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