-
Notifications
You must be signed in to change notification settings - Fork 14
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
MGDCTRS-1388: feat: Add custom component for boolean field #648
Conversation
indraraj
commented
Nov 18, 2022
...props | ||
}: CheckboxWithDescriptionProps) { | ||
return ( | ||
<Checkbox |
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 think we still need to wrap this in a FormGroup like this example, to be consistent with the other form controls. You can pass all the same properties as here. I guess if we add another custom component we may consider a wrapper function that injects this, or maybe we could go and expose the wrapper function that uniforms-patternfly uses for this.
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.
Sure I will add the "FormGroup", But I am not sure if we need all the same properties here as in Typeahead i.e label, type, isRequired may not be applicable here
@@ -77,6 +78,15 @@ export const JsonSchemaConfigurator: FunctionComponent<JsonSchemaConfiguratorPro | |||
component: TypeaheadField, | |||
}) | |||
: undefined; | |||
|
|||
forEach(otherProperties, (val: any, key: string) => { |
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 think this and the existing code for the typeahead field is good for now. If we start to have more custom components this will approach will require further development into some kind of very simple registry, also maybe there's ways we could get these pushed back to uniforms-patternfly.
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.
+1
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.
This looks great! I think it just needs the FormGroup component wrapping the checkbox and then it LGTM.
Added the FormGroup component wrapping |
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.
Tested locally, LGTM!