Added withTheme HOC

[danbalarin]

Reasons for making this change

Implemented withTheme HOC in order to be able to create custom theme without having to create forks. #1222

Example usage:

import React, { Component } from 'react'
import { withTheme } from 'react-jsonschema-form'
import Bootstrap4Theme from 'react-jsonschema-form-theme-bs4'

class Demo extends Component {
  render() {
    return withTheme(Bootstrap4Theme)
  }
}

Theme object can provide widgets, templates, fields and form in order to customize generated form.

Example Bootstrap 4 theme

Checklist

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• 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

[andronat]

@epicfaace any news regarding this? Very eager to see it in!

[epicfaace]

Looks good! Can you please add a page for documentation? I'm thinking you can add an entire new section called "Customizing with other frameworks" or something.

[epicfaace]

Can you rename restData to otherProps?

[danbalarin]

Sure, I'm very bad at naming variables.

[danbalarin]

So I added documentation, but I'm pretty bad at writing those things, so it would be nice if someone could look at it, fix typos, spelling errors and so on...

[epicfaace]

Thank you! Can you also add some tests? (using withTheme with a simple theme and making sure the corresponding widgets, templates, fields, etc. are used)

[danbalarin]

I can try it, but I've written one test in javascript so far. And I don't have a lot of time, so don't know when I will do it...

[epicfaace]

No problem, I can add the tests if you are not comfortable doing so.

[danbalarin]

That would be awesome, I have no idea how to test this functionality, so at least I would learn something new

[andronat]

Any news regarding this?

[TwoAbove]

Nice! Any news on this? Can't wait to implement the usage in my repo!

[epicfaace]

I'll be writing a test for this soon! Then will be ready to merge

[epicfaace]

@danbalarin I don't understand why the ability to pass in a custom form component to the theme is needed. Would there really be different kinds of data handling that each theme would require?

[danbalarin]

Umm, actually I used it in Bootstrap 4 since there might be needed to add was-validated class after validation. This is because when you have some validation made by HTML5 input itself (number, email, regex,...) when you submit the form, it's browser dependent how it shows the error (Firefox shows error input slightly differently from Chromium) and therefore inconsistent. But by providing custom <form>, there comes other issues like remembering the data filled in and with each submit refill all inputs (the stuff default form does...).

[epicfaace]

@danbalarin Can you remove the custom form option for now and perhaps move that to a later PR? I'm not convinced that a custom form is the right way to go, because then it seems like it involves too much customization; one can pass in an arbitrary component (not even one based on react-jsonschema-form's Form) for form.

For the case you mentioned, maybe the option of "adding a class after validation" should be implemented as a feature of react-jsonschema-form itself?

[danbalarin]

I've removed custom form.

[epicfaace]

Thanks! Can you update the documentation too? Then we should be all good to go.

[danbalarin]

Thanks! Can you update the documentation too? Then we should be all good to go.

Done

[mirajp]

Question: Why require the templates be packaged into a temporary object when the underlying Form component itself does not do so? Think it'd be simpler/more direct to just allow template overrides be passed through the same way the other props are; otherwise, users of the ThemedForm would also need to package any custom field template overrides they want to perform on an instance-by-instance basis inside a makeshift "templates" object (thus diverging from how the native Form does it, and thereby requiring each theme provider to document this usage that differs from/contradicts the original docs).

I also don't think this HOC should limit itself to just widgets, fields(, and templates) and should pretty much allow for the passing through of all custom props (the data object). For instance, a "theme" object may want to turn off the native errors list via the showErrorList prop because users (at least mine) may not want a separate error list on the top of the form when each field template already has a custom error message shown under the field. Thus, it would be nice to simply create a themed Form by doing something like this:

const ThemedForm = withTheme({
    FieldTemplate: MyThemeFieldTemplate,
    ...,
    showErrorList: false,
    fields: MyThemeFields,
    widgets: MyThemeWidgets
});

I get that at this point this "withTheme" HOC really just overrides every property (not just what some people would deem "theme" related), but I think it would prove much more useful. Otherwise, any Form customizations would have to extend the native Form or HOC class component in order to attach nice-to-have defaults for props like "showErrorList".

Proposed tweaks to this PR:

function withTheme(themeProps) {
  return class extends Component {
    render() {
      let { fields, widgets, ...directProps } = this.props;
      fields = { ...themeProps.fields, ...fields };
      widgets = { ...themeProps.widgets, ...widgets };
      return (
        <Form
          {...themeProps}
          {...directProps}
          fields={fields}
          widgets={widgets}
        />
      );
    }
  };
}

You may also want to consider a deep merge of the themeProps and the directProps to account for theme providers that want to provide defaults for other object-type properties (e.g. formContext or customFormats).

[epicfaace]

@mirajp good idea. What do you mean when you said that "templates [are] packaged into a temporary object"?

[mirajp]

@epicfaace based on how the HOC function is currently designed in this PR:

function withTheme(data) {
  return class extends Component {
    render() {
      let { templates, widgets, fields, ...otherProps } = this.props;
      templates = { ...data.templates, ...templates };
      widgets = { ...data.widgets, ...widgets };
      fields = { ...data.fields, ...fields };
      return (
        <Form
          {...otherProps}
          {...templates}
          widgets={widgets}
          fields={fields}
        />
      );
    }
  };
}

templates has to be a special object (keyed "templates") in the data object passed into the HOC function, and likewise instances of this themed Form would need to pass in a similar object to override any specific template:

const ThemedForm = withTheme({
    fields: {...}
    templates: {ArrayFieldTemplate: ThemedArrayFieldTemplate},
    widgets: {...},
});

<ThemedForm
    ...
    templates={{ ArrayFieldTemplate: MySpecialArrayFieldTemplate }}
/>

However, the actual rjsf Form has no such special templates object as a prop; instead, any field template override is done directly as a props/jsx attribute: https://react-jsonschema-form.readthedocs.io/en/latest/advanced-customization/#array-field-template

<Form
    ...
    ArrayFieldTemplate={MySpecialArrayFieldTemplate}
    ObjectFieldTemplate={MySpecialObjectFieldTemplate}
/>

Note that in the native rjsf Form, only fields, widgets, customFormats, and formContext are object-type properties/jsx attributes. Thus, with my proposed tweak in my original comment, the proposed withTheme({ ... }) HOC would allow more props to fall through (including all the *FieldTemplate props).

If the implementation was even less heavy-handed/opinionated (currently only merges together the fields and widgets objects) and did a deep merge on all object properties from both the data props of withTheme (I think this should be renamed to themeProps) and the form instance props (this.props), you could even allow themes to specify their own formContext and customFormats values that developers could amend on an instance-by-instance basis if any configs needed to be stored there for any runtime logic performed in the custom/themed widgets, etc.

[epicfaace]

@mirajp that's a good point. The templates, etc. structure of withTheme was based off of this PR (#1013), but since that PR is probably not going to be merged at this point, and I don't see the structure of the form props changing in the near future, it might make more sense to make it like what you discussed. Would you be able to modify this PR to have that behavior?

[mirajp]

@epicfaace I don't think I can modify this PR since I'm not a collaborator of this repo nor an authorized contributor of @danbalarin's master branch that this PR is merging from. All I can independently do is make a PR against his master branch or against rjsf's master branch -- may be simpler to just wait for @danbalarin to check back and copy and paste my snippet.

[danbalarin]

Originally I made it this way so you can define theme in a separate file and use default export. An object of this size is negligible because all it does is holding three pointers, so is passing it as a param, its only passing pointer not the whole object.

@mirajp I'm sorry, but I have a lot of work that needs to be done, I added you as a collaborator, so you can add proposed changes.

[mirajp]

@danbalarin I believe you should still be able to still the default export on the Theme options/props object, just this object will have all the field templates not nested under a templates object (I think the # of pointers would actually decrease (by 1 -- no pointer to a new templates object) and # of objects would also decrease (by 1 -- no templates object) :P -- though this is pretty insignificant in the grand scheme of things). I can review your branch and modify the PR and confirm your tests still work when I get a chance later today.

[mirajp]

@danbalarin @epicfaace updated the PR.

[epicfaace]

Thanks for the changes! Nearly there, just a few doc changes.

[epicfaace]

Thanks -- can you also add a few more test cases?

[epicfaace]

Can you add some more tests here that test the widgets, templates, and also the overriding?

[mirajp]

How do I even get the tests to run with changes? They keep running without any of my changes to the same file lol... (and only runs this one file, none of the other test files) I can literally delete all the tests and it still runs it - is there some weird build step involved and it only runs whatever got built?
I think I got it to work once by running cs-format and cs-check, but can't get npm run test to pick up any new changes to the file.

[mirajp]

@epicfaace added a few tests. I tried to get a custom ObjectFieldTemplate to render, but it never worked/was never called by Form =/ dunno why.

[epicfaace]

Looks good. Thanks for your hard work @mirajp @danbalarin !

It would be good @mirajp if you can look into why the ObjectFieldTemplate is not rendering in your test. Perhaps you can push your test to a separate branch / PR? I will merge this anyway for now.