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 withTheme HOC #1226

Merged
merged 15 commits into from
May 25, 2019
Merged

Added withTheme HOC #1226

merged 15 commits into from
May 25, 2019

Conversation

danbalarin
Copy link
Contributor

@danbalarin danbalarin commented Mar 18, 2019

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'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
Copy link

andronat commented Mar 20, 2019

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

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

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.

src/withTheme.js Outdated
function withTheme(data) {
return class extends Component {
render() {
let { templates, widgets, fields, ...restData } = this.props;
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 rename restData to otherProps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm very bad at naming variables.

@danbalarin
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@danbalarin
Copy link
Contributor Author

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

@andronat
Copy link

andronat commented Apr 3, 2019

Any news regarding this?

@TwoAbove
Copy link

TwoAbove commented Apr 4, 2019

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

@epicfaace
Copy link
Member

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

@epicfaace
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

I've removed custom form.

@epicfaace
Copy link
Member

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

@danbalarin
Copy link
Contributor Author

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

Done

@mirajp
Copy link
Contributor

mirajp commented May 19, 2019

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
Copy link
Member

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

@mirajp
Copy link
Contributor

mirajp commented May 19, 2019

@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
Copy link
Member

@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
Copy link
Contributor

mirajp commented May 19, 2019

@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
Copy link
Contributor Author

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
Copy link
Contributor

mirajp commented May 20, 2019

@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
Copy link
Contributor

mirajp commented May 21, 2019

@danbalarin @epicfaace updated the PR.

mkdocs.yml Outdated Show resolved Hide resolved
Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

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

Co-Authored-By: Ashwin Ramaswami <aramaswamis@gmail.com>
Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

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

sandbox.restore();
});

describe("With widgets", () => {
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 some more tests here that test the widgets, templates, and also the overriding?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

test/withTheme_test.js Outdated Show resolved Hide resolved
mirajp and others added 2 commits May 23, 2019 22:10
Co-Authored-By: Ashwin Ramaswami <aramaswamis@gmail.com>
Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

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.

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.

5 participants