Button - plain progress should support passing icon as children

[mcoker]

In core, because of the way we want to manage the spacing for the animation of the progress indicator and button text in non-plain buttons between the in progress and not in progress states, we have a .pf-m-progress class that changes the padding a bit to allow for a smooth animation. From there, the progress button then gets .pf-m-in-progress dynamically to toggle between in progress and not in progress. The plain button doesn't have any sort of spacing or button text animation, so it only needs the .pf-m-in-progress class. Adding .pf-m-progress applies the animation padding to the plain button, which doesn't look correct.

Currently in the react component, a plain progress button requires either of the following when it is not yet in the "in progress" (isLoading) state, otherwise the styles.modifiers.progress class is added, which applies the wrong padding.

• if the icon is passed as children, it needs isLoading === null - setting it to false adds styles.modifiers.progress
  • seems like in apps there will likely be a bool state var that manages the progress state of whatever's going on in the page (eg, dataRefreshing), and it would be great just to use isLoading={dataRefreshing}, but that doesn't work if {dataRefreshing} is false.
• for the icon passed as the icon prop, and for children === null

I think we can address this one of a couple of ways:

• address this in the CSS, and disable any of the .pf-m-progress styles from applying to to the plain button. Then the isLoading prop on the <Button> component can work the same between plain and non-plain buttons.
• Update this line

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

  Line 158 in 9eacaae

  isLoading !== null && children !== null && styles.modifiers.progress,

  
  to something like

   isLoading !== null && variant !== ButtonVariant.plain && styles.modifiers.progress,
  

Or maybe some other way? I see pros/cons to either approach, curious your thoughts @tlabaj @nicolethoen, also @thatblindgeye since you worked on the inline progress button (which needs .pf-m-progress)

[nicolethoen]

I think the react update makes sense to me. I don't understand the cons of the react update, though - what are they?

[mcoker]

@nicolethoen I think these are the main 2 points to make the update in core.

• It's more consistent to use .pf-m-progress as a button that is a "progress" button, and .pf-m-in-progress for any button that's in progress. You can argue they're meaningful classes as opposed to just providing styling.
• If the plain button (or others that don't need .pf-m-progress) do end up needing that class for some kind of styling, it's just a CSS update that users get for free as opposed to also requiring a markup update, too - react component update for pf-react, HTML update for core users.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.