-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adding ObjectControl #26
Conversation
() => | ||
isEmpty(path) | ||
? Generate.uiSchema(schema, "VerticalLayout") | ||
: { ...Generate.uiSchema(schema, "Group"), label }, |
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 migrated the GroupRenderer since I think this needs that
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. We can revisit the naming conventions later if we want.
if (!visible) { | ||
return null | ||
} |
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.
nit: I've been moving these to the top so you don't call findUISchema
.
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.
Since findUISchema is within a react hook it cannot be conditional.
src/layouts/GroupRenderer.tsx
Outdated
elements: UISchema[] | ||
} | ||
|
||
export function GroupComponent({ visible, enabled, uischema, ...props }: LayoutRendererProps) { |
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.
Trying to wrap my head around these layouts
. Why is this called GroupComponent
but the file is called GroupRenderer
? I don't love the names "component' or "renderer" TBH. What would you think if we called them both GroupLayout
and aliased the jsonforms interface?
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.
That sounds good. Not sure why it was named that before, but I just ported the naming over.
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.