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

Added ui:*Template properties #1152

Merged
merged 8 commits into from
Jul 13, 2019
Merged

Added ui:*Template properties #1152

merged 8 commits into from
Jul 13, 2019

Conversation

loganvolkers
Copy link
Contributor

Reasons for making this change

Makes customizing templates for a form a more granular experience.

Resolves #1010

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

…CRLF endings to LF to stop Husky complaining.
@loganvolkers
Copy link
Contributor Author

@epicfaace pull request for adding ui:template.

Would be interested in some feedback on the naming convention. Since an array template has both a SchemaField template and an ArrayField template nested inside, it wasn't possible to have a single ui:template property, as it is ambiguous which template needs to be overridden.

Instead I opted to follow the naming convention for overriding fields globally:

  • ui:FieldTemplate for all fields
  • ui:ArrayFieldTemplate for the array field subtemplates
  • ui:ObjectFieldTemplate for the object field subtemplates

I found this the most consistent with the current API. In the future it would be handy if the templating system could be consolidated in a way that a single ui:template field would make sense, but that's a substantial non-backwards-compatible undertaking.

This is I think the best compromise between allowing granular overriding of field templates and respecting existing behaviour and API design.

test/FieldTemplate_test.js Outdated Show resolved Hide resolved
DescriptionField,
},
}).node;
sharedIts();
});
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test in which a template is globally configured and a template is configured in ui:ObjectFieldTemplate so it has to override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

test/FieldTemplate_test.js Show resolved Hide resolved
uiSchema,
}).node;
});
sharedIts();
});
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test in which a template is globally configured and a template is configured in ui:ArrayFieldTemplate so it has to override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

uiSchema,
}).node;
});
sharedIts();
});
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test in which a template is globally configured and a template is configured in ui:ArrayFieldTemplate so it has to override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

package.json Show resolved Hide resolved
@epicfaace
Copy link
Member

epicfaace commented Jan 22, 2019

  • ui:FieldTemplate for all fields
  • ui:ArrayFieldTemplate for the array field subtemplates
  • ui:ObjectFieldTemplate for the object field subtemplate

@loganvolkers that works. What I'm thinking, though, is what if we have our uiSchema in JSON and want to reference "ui:FieldTemplate", "ui:arrayFieldTemplate", or "ui:ObjectFieldTemplate" through a string, not a React component? The way it's done right now for ui:widget and ui:field is through the widgets and fields props on the Form, respectively.

Is the only way to do this to define fieldTemplates, arrayFieldTemplates, and objectFieldTemplates props on the Form as well? That seems like it will clutter the props a lot, especially given that we already have props called FieldTemplate, ArrayFieldTemplate, and ObjectFieldTemplate.

Here's my proposal. On the other hand, if we just use ui:template for all three (and don't let FieldTemplate be customized for array fields), then we have a nice prop called templates that we can use for this purpose. If we really want to customize FieldTemplate for array fields, maybe we can allow ui:FieldTemplate just for array fields, but it will also read from the templates prop.

Any thoughts?

@loganvolkers
Copy link
Contributor Author

What I'm thinking, though, is what if we have our uiSchema in JSON and want to reference "ui:FieldTemplate", "ui:arrayFieldTemplate", or "ui:ObjectFieldTemplate" through a string
then we have a nice prop called templates that we can use for this purpose

I agree with this. templates could be added to the registry and use the same capabilities that exist for string-referenced widgets and fields.

e.g. for an object field:

<Form
  {...props}
  templates={{"tabs":MyTabTemplate}}
  uiSchema={{"ui:ObjectFieldTemplate":"tabs"}}
/>

and don't let FieldTemplate be customized for array fields

This is a non-starter for me. The goal is to provide UI customization and there is a lot of template-related stuff in the FieldTemplate, especially for Object fields with the new oneOf support.

@epicfaace
Copy link
Member

@loganvolkers that sounds good, using ui:ArrayFieldTemplate, ui:FieldTemplate, and ui:ObjectFieldTemplate and then adding support for the templates prop in a later PR. For now, can you add the additional tests I mentioned and update the docs? -- and then I think it'll be ready to be merged.

@epicfaace
Copy link
Member

@loganvolkers will you have time to make these changes? I'd like to merge this PR

@epicfaace
Copy link
Member

@loganvolkers just bumping this!

@DavidHenri008
Copy link

@loganvolkers any update on this? This looks really promising.

@loganvolkers
Copy link
Contributor Author

Working on wrapping this up.

I just made some progress on merging the latest to bring this branch up to speed, but there are still some changes requested for tests and docs.

@loganvolkers
Copy link
Contributor Author

@epicfaace can you please review? I believe this should meet your requests.

@epicfaace
Copy link
Member

Thanks @loganvolkers looks good -- can you update the docs?

@loganvolkers
Copy link
Contributor Author

@epicfaace merged latest changes and updated docs. Please take a look.

@epicfaace
Copy link
Member

Looks good -- thanks!

@epicfaace epicfaace merged commit 310b8a5 into rjsf-team:master Jul 13, 2019
@timkindberg
Copy link

timkindberg commented Aug 7, 2019

I agree with this. templates could be added to the registry and use the same capabilities that exist for string-referenced widgets and fields.

Was this piece implemented?

@loganvolkers
Copy link
Contributor Author

loganvolkers commented Aug 7, 2019 via email

@epicfaace
Copy link
Member

I agree with this. templates could be added to the registry and use the same capabilities that exist for string-referenced widgets and fields.

Was this piece implemented?

It's now being tracked in #1387, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"ui:template" schema option
4 participants