Separate templates and allow a completely custom UI components #1013

MatejBransky · 2018-08-26T10:19:31Z

Reasons for making this change

This PR separates template components from fields so you can replace all UI components with your own.

New API:

Form.props.templates - mainly UI components separated from fields
- templates.ArrayFieldTemplate - old ArrayFieldTemplate
- templates.DescriptionTemplate - old fields.DescriptionField
- templates.ErrorListTemplate - old ErrorList
- templates.FieldTemplate - old FieldTemplate
- templates.ObjectFieldTemplate - old ObjectFieldTemplate
- templates.SubmitTemplate - old default submit button in Form's render
- templates.TitleTemplate - old fields.TitleField
added HOC withTheme - it wraps Form with components (fields, widgets, templates), it's useful for building a custom theme:

import withTheme from 'react-jsonschema-form/lib/components/withTheme';

import widgets from './my-theme/widgets';
import templates from './my-theme/templates';

const Form = withTheme('MyTheme', { widgets, templates });

Fields, widgets, templates...this diagram helped me to keep the basic relationships between components in my head (ArrayField is extended in readme):

And it's quite useful if you build your custom UI.

Breaking changes (I suppose that this PR should land in v2):

replaced Form.props.FieldTemplate with Form.props.templates.FieldTemplate
replaced Form.props.ArrayFieldTemplate with Form.props.templates.ArrayFieldTemplate
replaced Form.props.ObjectFieldTemplate with Form.props.templates.ObjectFieldTemplate
replaced Form.props.ErrorList with Form.props.templates.ErrorListTemplate
replaced Form.props.fields.DescriptionField with Form.props.templates.DescriptionTemplate
replaced Form.props.fields.TitleField with Form.props.templates.TitleTemplate
all "raw-" props replace component based versions
- rawErrors is now errors
- rawDescription is now description
- rawHelp is now help

If this is related to existing tickets, include links to them as well.

#693 - build whatever UI you want
#766 - <p> tags replaced with <div> in SubmitTemplate
#767 - uppercased props moved into the templates
#899 - this merge allows relatively painless update from Bootstrap 3 to Bootstrap 4 (I have prepared update to Bootstrap 4 based on this PR) and if anyone wants to use a custom theme without importing Bootstrap 4 components then with this merge it's finally possible
#951, #952 - FieldTemplate is separated
#1010 - templates

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
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

MatejBransky · 2018-08-26T10:54:06Z

I had an old fork so I had to solve some merge issues and some of them I overlooked and merged them incorrectly. Now it should be ok (tests pass).

loganvolkers · 2018-08-28T18:31:16Z

src/components/Form.js

-      FieldTemplate: this.props.FieldTemplate,
+      fields: this.props.fields,
+      widgets: this.props.widgets,
+      templates: this.props.templates,


To maintain backwards compatibility with current props, you could check for the old props here and use them if they were provided.

Otherwise to be strict with Semver including this PR would mean releasing a V2.

If we include backwards compatible props, then this could be released as V1.1.

For example (this is pseudocode), this could provide backwards compatibility

templates: { ...this.props.templates, ArrayFieldTemplate: this.props.ArrayFieldTemplate || this.props.templates.ArrayFieldTemplate, ObjectFieldTemplate: this.props.ObjectFieldTemplate || this.props.templates.ObjectFieldTemplate, FieldTemplate: this.props.FieldTemplate || this.props.templates.FieldTemplate, ErrorListTemplate: this.props.ErrorList || this.props.templates.ErrorListTemplate }

@glasserc do you think that maintaining backwards compatibility with props is important?

I thought that this would be in v2 (I've updated the first comment with this info) but yes we can change it to support backward compatibility.

loganvolkers · 2018-08-28T18:35:31Z

src/components/fields/ArrayField.js

      onBlur,
      onFocus,
      idPrefix,
-      rawErrors,
+      errors,


I don't see the benefit that renaming errors to rawErrors would provide.

I understand that the naming would be more clear, but the downside is that this introduces breaking changes.

Avoiding breaking changes means that this PR is easier to merge, easier to release and more people will get benefits earlier.

I wanted to mainly avoid adding another template (ErrorList from old SchemaField). Because component based errors needs UI component ErrorList which was in the old SchemaField.js. But sure we can take that back it probably means that I need to create another template for this ErrorList. What would be its name? ErrorListTemplate is taken by the template for list of all errors visible at the top of the form. So it should be the name which doesn't confuse devs.

I don't really understand this argument. What are we using this errors and how is it different from rawErrors that we had before?

src/components/fields/SchemaField.js

loganvolkers · 2018-08-28T18:42:55Z

@matejmazur I added some comments to your PR.

I have a feeling that there isn't really a requirement to have backwards-incompatible changes as part of this.

I also have a feeling that if this doesn't introduce backwards-incompatible changes, then it will be far easier to merge and release, and make Ethan's life easier.

MatejBransky · 2018-08-28T19:03:50Z

@loganvolkers Thanks for your review! 👍
Backward compatibility is possible. I just didn't keep it in mind.

Fedorov113 · 2018-10-01T19:13:58Z

Any update on this?

MatejBransky · 2018-10-01T20:33:16Z

@Fedorov113
Unfortunately no, because nobody from contributors has responded yet (they are probably not interested into this or they don't have any time to review it).

By the way it was for me a lot of work to refactor it such a way that is just an improvement without any huge changes (I've tried multiple refactorings before this PR)..and still no response from maintainers. :-/

But at least I've learned something about forms and I've found out that it's one of the hardest parts in frontend...you need to solve so many edge cases. This library and JSON Schema helped me with it a lot. So huge thanks belong to all contributors who tried to keep it alive.

For now it's easier for me to use a private version with custom enhancements.

HerbCaudill · 2018-10-03T09:10:33Z

Hi - I've been following this conversation from the beginning, because I think rjsf is amazing but would like to use something besides Bootstrap.

It seems clear for me that the future of this project has to be framework-independent. I've read through #623 and #899 etc. etc. and I understand that while @glasserc and other maintainers agree in principle, they don't have the time to make it happen.

@matejmazur seems to have gotten us 99% of the way there, but with breaking changes. So I wonder what the right way to move this forward is. Is there any interest in a v2? The only mention I've seen of that, outside of this PR, is #805. Or is it just a matter of addressing @loganvolkers's comments above to make these changes backwards-compatible?

glasserc · 2018-10-03T19:45:39Z

Hi, sorry for taking so long to respond. In principle, what I would like to see is a move from "import rjsf and you get Bootstrap for free" to "import rjsf and one of the helper libraries; if you want Bootstrap, you have to say so explicitly". Fundamentally that's a breaking change and would have to be a major version. Even just changing from Bootstrap v3 to Bootstrap v4 feels like a breaking change to me. So I don't conceptually have a problem with other breaking changes happening at the same time.

I also don't have a good answer for what the way forward is. I also thought that this PR was the brightest path, but I guess I took too long to review it and now it's closed. I took a quick look before writing this comment and a lot of things are changing in this PR all at once, which makes it a bit hard to review. I tried to review commit-by-commit but since most of the changes are in the first commit, that doesn't really help. I sympathize with @matejmazur's difficulty in making a change that is both easily-reviewable and valuable.

It seems like fundamentally, most of the pieces are already in place -- we already have DescriptionField and TitleField. That's good and mostly what I expected. I guess mostly what we need is some cleanups around defining "themes" and extraction of the default BSv3 theme. I really do intend to review such a PR because it beats having to write it myself, but as noted, I can only put a limited amount of effort on the maintenance of rjsf and a lot of it gets sucked up by smaller issues.

MatejBransky · 2018-10-03T21:19:24Z

Finally! Glad to see your response, @glasserc ! 🙂 I understand that it's a huge change. Sorry about that but I did this in a rush (from a different repo).
Unfortunately I don't have time right now but if you could bring up some ideas how would you like to proceed next...I'll be glad for that.

Btw refactoring Bootstrap from 3 to 4 is the easiest part (it just depends on the separating of all UI components like is, for example, proposed in this PR).

This PR basically just move all *Template components from *Field components to separate files.

HerbCaudill · 2018-10-04T08:43:19Z

what I would like to see is a move from "import rjsf and you get Bootstrap for free" to "import rjsf and one of the helper libraries; if you want Bootstrap, you have to say so explicitly". Fundamentally that's a breaking change and would have to be a major version.

That makes sense.

The alternative - "import rjsf and you get Bootstrap for free, unless you override it with your UI framework of choice" - seems reasonable as well, and would make the upgrade path to V2 a bit easier.

Along those lines, it seems like even if this effort results in a V2, @loganvolkers's suggestions for renaming things etc. to make it backwards-compatible would make it easier for people to upgrade. And an easy upgrade would mean less pressure to continue maintaining V1 in parallel.

I'd be happy to work on this - @matejmazur / @glasserc , let me know what you think would be helpful to move this forward.

glasserc

I kind of agree with your point of view @HerbCaudill. Maybe the first step is to separate out the parts of the PR that add the functionality and organization we want, and merge that, with a remaining PR to do the restructuring of arguments and such.

I think the following things are pretty unobjectionable:

Fixes to typos in the README.
Extracting template classes into src/components/templates/. I think this is the biggest change by lines of code touched so separating this out would make reviewing the rest a lot easier.
Renaming classes that aren't exposed externally, e.g. TitleField to TitleTemplate. I think this reinforces the division of fields, widgets, and templates without causing any API breakage. On the other hand, maybe we want to keep these in sync with the prop names?
Adding the withTheme function. This might be helpful because we can introduce the new API here and maintain backwards compatibility here without tampering with the existing Form class.

Less clear to me:

We should add any new templates we need. I think this is only SubmitTemplate. How do we let a user pass SubmitTemplate? Do we start using this templates prop or do we rely on putting everything in the Form props for consistency until v2?
Internal reorganizations such as passing TitleField and DescriptionField through the registry instead of explicitly. I guess there's nothing wrong with it but maybe we should separate this change and review it by itself.

Things that would maybe wait until later:

Changing API to rely on a templates prop. We could make the change internally if we added backwards compatibility though.

I'd also love to see an example of using whatever API with another UI library such as Material. I'm not sure how much would have to change to use a different library. I'm nervous about how much we'd have to duplicate in e.g. widgets.

glasserc · 2018-10-04T20:53:58Z

src/components/widgets/index.js

@@ -38,4 +89,5 @@ export default {
  FileWidget,
  CheckboxWidget,
  CheckboxesWidget,
+  __widgetMap: widgetMap,


What's this change about?

glasserc · 2018-10-04T21:00:43Z

README.md

@@ -1544,7 +1570,7 @@ Here are some examples from the [playground](http://mozilla-services.github.io/r
 ![](http://i.imgur.com/IMFqMwK.png)
 ![](http://i.imgur.com/HOACwt5.png)

-Last, if you really really want to override the semantics generated by the lib, you can always create and use your own custom [widget](#custom-widget-components), [field](#custom-field-components) and/or [schema field](#custom-schemafield) components.
+Last, if you really really want to override the semantics generated by the lib, you can always create and use your own custom [widget](#custom-widget-components), templates, [field](#custom-field-components) and/or [schema field](#custom-schemafield) components.


Shouldn't there be a link for templates?

glasserc · 2018-10-04T21:01:03Z

README.md


-A: There's no specific built-in way to do this, but you can write your own FieldTemplate that supports hiding/showing fields according to user input. We don't yet have an example of this use, but if you write one, please add it to the "tips and tricks" section, above. See also: [#268](https://github.com/mozilla-services/react-jsonschema-form/issues/268), [#304](https://github.com/mozilla-services/react-jsonschema-form/pull/304), [#598](https://github.com/mozilla-services/react-jsonschema-form/issues/598), [#920](https://github.com/mozilla-services/react-jsonschema-form/issues/920).


Shouldn't we keep the collapse Q?

glasserc · 2018-10-04T21:07:53Z

src/components/fields/ArrayField.js

      onBlur,
      onFocus,
      idPrefix,
-      rawErrors,
+      errors,


I don't really understand this argument. What are we using this errors and how is it different from rawErrors that we had before?

glasserc · 2018-10-04T21:22:28Z

playground/app.js

@@ -415,6 +412,7 @@ class App extends Component {
            </div>
            <div className="col-sm-2">
              <Form
+                idPrefix="live-validate"


What's this change about?

glasserc · 2018-10-04T21:22:53Z

src/components/Form.js


    if (errors.length && showErrorList != false) {
      return (
-        <ErrorList
+        <ErrorListTemplate


Why does this need to be a parameter now instead of coming from props?

glasserc · 2018-10-04T21:26:41Z

src/components/fields/SchemaField.js

+    <FieldTemplate {...templateProps}>
+      <Field {...fieldProps} />
+    </FieldTemplate>
+  );


What are these changes about?

Fedorov113 · 2018-10-29T23:33:12Z

Sorry for bumping again, but @matejmazur haven't you look further into this?

Fedorov113 · 2018-11-01T10:47:05Z

So, I,ve started implementing React Material UI forms using @matejmazur approach. The process is pretty straightforward and works like a charm. I really hope this great PR will be merged into master somewhere in the near future.

rodrigopavezi · 2018-11-27T01:31:24Z

Is there any chance this PR will be merged soon? We really need this. Cheers

rodrigopavezi · 2018-11-27T02:37:15Z

@Fedorov113 could you share what you have done so far with material-ui? Cheers

mnlbox · 2019-01-14T12:33:37Z

Any news?
I need use this component with material-ui and also if possible with react-native. 🤔

epicfaace · 2019-01-20T23:01:53Z

I think the following things are pretty unobjectionable:

Fixes to typos in the README.

Extracting template classes into src/components/templates/. I think this is the biggest change by lines of code touched so separating this out would make reviewing the rest a lot easier.

Renaming classes that aren't exposed externally, e.g. TitleField to TitleTemplate. I think this reinforces the division of fields, widgets, and templates without causing any API breakage. On the other hand, maybe we want to keep these in sync with the prop names?

Adding the withTheme function. This might be helpful because we can introduce the new API here and maintain backwards compatibility here without tampering with the existing Form class.

@MatejBransky Are you willing to open another PR with the "unobjectionable" changes of this pull request mentioned by @glasserc ? Then we should have no problem with merging it.

sbusch · 2019-02-26T10:49:38Z

This PR should be included in V2 project IMO.

videni · 2019-07-15T02:35:57Z

@MatejBransky，how is this going? I really like this PR to be merged.

epicfaace · 2019-09-09T05:08:11Z

I'm closing this because we already have a withTheme component, though if you'd like for any of the additional changes to be added to a future version (like v2), let me know.

Matej Mazur added 6 commits August 26, 2018 12:14

Separate templates

838ef01

Update readme

adde800

Fix merge issue

50f20da

Fix merge issue (utils.js)

af03462

Fix merge issue (Form_test.js)

683f073

Update readme

c37e850

Matej Mazur added 7 commits August 26, 2018 13:07

Update diagram

8c59b3c

Update diagram: remove icons (GitHub probably doesn't support them)

7180a69

Update readme: fix typo

1bb2f9c

Fix idPrefix in playground

e598a23

Fix propTypes for help and description

9df47ad

Update diagrams

f37be97

Update gitignore

a573096

loganvolkers reviewed Aug 28, 2018

View reviewed changes

src/components/fields/SchemaField.js Show resolved Hide resolved

MatejBransky force-pushed the templates branch from 0b87e63 to a573096 Compare August 29, 2018 10:09

MatejBransky mentioned this pull request Sep 4, 2018

Bug with setState in event handlers #1023

Closed

2 tasks

MatejBransky closed this Oct 1, 2018

MatejBransky reopened this Oct 3, 2018

MatejBransky mentioned this pull request Oct 4, 2018

Rename some props to be lowercase #767

Closed

4 tasks

glasserc reviewed Oct 4, 2018

View reviewed changes

epicfaace mentioned this pull request Feb 11, 2019

Should we move to Bootstrap v4? #899

Closed

epicfaace added breaking change blocked labels Feb 26, 2019

epicfaace mentioned this pull request Mar 16, 2019

Create a withTheme higher-order component #1222

Closed

epicfaace mentioned this pull request May 19, 2019

Added withTheme HOC #1226

Merged

8 tasks

epicfaace closed this Sep 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate templates and allow a completely custom UI components #1013

Separate templates and allow a completely custom UI components #1013

MatejBransky commented Aug 26, 2018 •

edited

Loading

MatejBransky commented Aug 26, 2018

loganvolkers Aug 28, 2018 •

edited

Loading

MatejBransky Aug 28, 2018 •

edited

Loading

loganvolkers Aug 28, 2018

MatejBransky Aug 28, 2018

glasserc Oct 4, 2018

loganvolkers commented Aug 28, 2018

MatejBransky commented Aug 28, 2018 •

edited

Loading

Fedorov113 commented Oct 1, 2018

MatejBransky commented Oct 1, 2018 •

edited

Loading

HerbCaudill commented Oct 3, 2018

glasserc commented Oct 3, 2018

MatejBransky commented Oct 3, 2018 •

edited

Loading

HerbCaudill commented Oct 4, 2018

glasserc left a comment

glasserc Oct 4, 2018

glasserc Oct 4, 2018

glasserc Oct 4, 2018

glasserc Oct 4, 2018

glasserc Oct 4, 2018

glasserc Oct 4, 2018

glasserc Oct 4, 2018

Fedorov113 commented Oct 29, 2018

Fedorov113 commented Nov 1, 2018

rodrigopavezi commented Nov 27, 2018

rodrigopavezi commented Nov 27, 2018

mnlbox commented Jan 14, 2019 •

edited

Loading

epicfaace commented Jan 20, 2019

sbusch commented Feb 26, 2019

videni commented Jul 15, 2019

epicfaace commented Sep 9, 2019


		A: There's no specific built-in way to do this, but you can write your own FieldTemplate that supports hiding/showing fields according to user input. We don't yet have an example of this use, but if you write one, please add it to the "tips and tricks" section, above. See also: [#268](https://github.com/mozilla-services/react-jsonschema-form/issues/268), [#304](https://github.com/mozilla-services/react-jsonschema-form/pull/304), [#598](https://github.com/mozilla-services/react-jsonschema-form/issues/598), [#920](https://github.com/mozilla-services/react-jsonschema-form/issues/920).

Separate templates and allow a completely custom UI components #1013

Separate templates and allow a completely custom UI components #1013

Conversation

MatejBransky commented Aug 26, 2018 • edited Loading

Reasons for making this change

Checklist

MatejBransky commented Aug 26, 2018

loganvolkers Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

MatejBransky Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganvolkers commented Aug 28, 2018

MatejBransky commented Aug 28, 2018 • edited Loading

Fedorov113 commented Oct 1, 2018

MatejBransky commented Oct 1, 2018 • edited Loading

HerbCaudill commented Oct 3, 2018

glasserc commented Oct 3, 2018

MatejBransky commented Oct 3, 2018 • edited Loading

HerbCaudill commented Oct 4, 2018

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fedorov113 commented Oct 29, 2018

Fedorov113 commented Nov 1, 2018

rodrigopavezi commented Nov 27, 2018

rodrigopavezi commented Nov 27, 2018

mnlbox commented Jan 14, 2019 • edited Loading

epicfaace commented Jan 20, 2019

sbusch commented Feb 26, 2019

videni commented Jul 15, 2019

epicfaace commented Sep 9, 2019

MatejBransky commented Aug 26, 2018 •

edited

Loading

loganvolkers Aug 28, 2018 •

edited

Loading

MatejBransky Aug 28, 2018 •

edited

Loading

MatejBransky commented Aug 28, 2018 •

edited

Loading

MatejBransky commented Oct 1, 2018 •

edited

Loading

MatejBransky commented Oct 3, 2018 •

edited

Loading

mnlbox commented Jan 14, 2019 •

edited

Loading