Skip to content
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(FormGroup): rename labelIcon to labelHelp #581

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

adamviktora
Copy link
Contributor

@adamviktora adamviktora commented Feb 20, 2024

Closes #563

Wait until the underlying react PR patternfly/patternfly-react#10016 is approved and merged before merging this codemod PR.

Possible improvements / thoughts:

  • trying to adjust whatever consumer is passing to labelIcon to use the new FormGroupLabelHelp component is nearly impossible and there would be lots of edge cases, so I would rather not do that
  • can we add a comment directly to the code, which would recommend using FormGroupLabelHelp component for the labelHelp prop? Something like:
    input:
<FormGroup labelIcon={<button>Help icon</button>} />

output:

<FormGroup labelHelp={/* CODEMODS RECOMMENDATION: use FormGroupLabelHelp component */ <button>Help icon</button>} />

or is a message in the codemod error sufficient?

Copy link
Collaborator

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

One comment below. Agreed that we shouldn't try to update code to render FormGroupLabelHelp, that should be on the consumer once they're made aware of the update + our recommendation.

As for adding a comment inline with wherever the labelHelp prop is, I'd probably lean towards not doing that, since it's clutter being added to consumer code that may not be necessary. The consumer could already have their own component being passed that is essentially our FormGroupLabelHelp or something, and the error message will already recommend it which I think should be good.

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

One nit and agree with @thatblindgeye's thoughts above, nothing blocking that I see though.

@thatblindgeye thatblindgeye merged commit 61f97d0 into patternfly:main Feb 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FormGroup - Penta updates
3 participants