-
Notifications
You must be signed in to change notification settings - Fork 56
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: allow passing custom element to field options #1880
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1880 +/- ##
=======================================
Coverage 95.26% 95.27%
=======================================
Files 52 52
Lines 3445 3449 +4
Branches 501 503 +2
=======================================
+ Hits 3282 3286 +4
Misses 136 136
Partials 27 27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 would add a test where both renderer and element properties are present to verify that only the renderer is shown.
Otherwise LGTM
const result = await FormController.init( | ||
user, | ||
render( | ||
<AutoForm | ||
service={personService()} | ||
model={PersonModel} | ||
disabled | ||
fieldOptions={{ | ||
lastName: { | ||
renderer: ({ field }) => <TextArea key={field.name} {...field} />, | ||
}, | ||
}} | ||
/>, | ||
).container, | ||
); |
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.
Why are you not using populatePersonForm function?
const result = await FormController.init( | |
user, | |
render( | |
<AutoForm | |
service={personService()} | |
model={PersonModel} | |
disabled | |
fieldOptions={{ | |
lastName: { | |
renderer: ({ field }) => <TextArea key={field.name} {...field} />, | |
}, | |
}} | |
/>, | |
).container, | |
); | |
const result = await populatePersonForm( | |
1, | |
{ lastName: { renderer: ({ field }) => <TextArea key={field.name} {...field} /> } }, | |
undefined, | |
true, | |
); |
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.
Thanks, updated the tests and added one for checking that the renderer is used over the element.
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.
Based on the internal discussion over the usage of cloneElement
, LGTM.
Quality Gate failedFailed conditions 15.9% Duplication on New Code (required ≤ 3%) |
This ticket/PR has been released with Hilla 24.4.0.alpha1 and is also targeting the upcoming stable 24.4.0 version. |
#1876 added a number of common properties to the field options, however for a lot of other customizations you still need to implement a custom renderer function. To make customizations a bit easier this adds a new
element
prop to the field options that allows passing a pre-rendered React element. The element is then automatically bound to the form and other field option props are added to it.This is easier to use than the renderer as you don't need to figure out the function signature and it results in simpler code. For most use-cases this should be able to replace the renderer function.
The downside is that there would be two options doing similar things, and that you need to make sure you pass an element of a field that actually works with the property type. Maybe
renderer
could accept both an element or a function, but I felt a second prop would make sense here.Some examples follow.
Render a TextArea instead of a TextField
Show a clear button
Show a prefix
Fixes #1732