-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: The label prop for TextInput can now take a component (ReactElement) as well as string
#2991
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: The label prop for TextInput can now take a component (ReactElement) as well as string
#2991
Conversation
by running `yarn test -u`
p-syche
left a comment
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.
hi @smbadiwe , thank you very much for this change. It's great you were able to introduce a new functionality without breaking the old.
Your code is good, however I would love to see some PR improvements before approving. Namely I would love to see the new feature added to the TextInput examples and I would appreciate a test checking the function you added for checking label change.
p-syche
left a comment
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.
You wrote very extensive tests 💪 great job!
LGTM
|
I think the issue can be fixed on the |
|
label={ |



Summary
The PR provides a way to solve the problem being discussed at #2591. Currently there's no way to customize the label on
TextInput. A common use-case is adding a red asterisk to indicate that a field is required. This PR provides for thelabelfield to also accept a component, thereby solving the problem.Test Plan
Here's an example of how a custom component can be passed in:
Of course the old usage still works, so this is not a breaking change.