feat(HelperText): consumed Penta updates by thatblindgeye · Pull Request #10029

feat(HelperText): consumed Penta updates by thatblindgeye · Pull Request #10029 · patternfly/patternfly-react

Conversation

What: Closes #10005
Codemod: patternfly/pf-codemods#552

Per discussion with @lboehling removed the isDynamic prop has the dynamic modifier isn't needed. This would require an update in Core to remove that modifier and its styling. Also removed the hasIcon prop has a default icon will now render automatically for variants other than "default"

We'll also want to update Core examples

Additional issues:

Eric Olkowski added 4 commits

January 24, 2024 15:03

Choose a reason for hiding this comment

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

One non-blocking thought/potential discussion point but other than that looks great!

Comment on lines +9 to +14

		export enum HelperTextItemVariant {
		default = 'default',
		warning = 'warning',
		error = 'error',
		success = 'success'
		}

Choose a reason for hiding this comment

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

I feel like we use this same set of options in a good few places, I wonder if it would make sense for us to extract this kind of "status" enum/type into the constants file. Not a blocking thing, just a thought.

Choose a reason for hiding this comment

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

Though how each component uses these variants can be slightly different. Here it's these 4 variants, Alert has danger | success | warning | info | custom, Progress only has dange | success | warning, etc. So it could also be a reason to make components follow a more universal variant naming when we can. At the very least, re-evaluating whether "error" should be used instead of danger in some places or vice versa, and whether ___Variant or ___Status makes more sense as an enum/prop name.