-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(HelperText): consumed Penta updates #10029
Conversation
Preview: https://patternfly-react-pr-10029.surge.sh A11y report: https://patternfly-react-pr-10029-a11y.surge.sh |
bb80129
to
d7cb7b1
Compare
There was a problem hiding this comment.
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!
export enum HelperTextItemVariant { | ||
default = 'default', | ||
warning = 'warning', | ||
error = 'error', | ||
success = 'success' | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - created an issue to make the help icon button on form labels in examples to be the plain, no-padding button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The props are not showing up in props table, otherwise LGTM.
What: Closes #10005
Codemod: patternfly/pf-codemods#552
Helper text preview
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: