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

Add material-ui theme #1420

Merged
merged 5 commits into from
Sep 20, 2019
Merged

Add material-ui theme #1420

merged 5 commits into from
Sep 20, 2019

Conversation

agustin107
Copy link
Member

Reasons for making this change

Add the capability to use ´rjsf´ with the theme of ´material-ui´.

According to issue #1222.

Checklist

  • I'm adding a new feature
    • I've added a playground with example use of the feature

@@ -0,0 +1,21 @@
MIT License
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I'm not too familiar with having code with multiple licenses in a single project, so I have a question. Does the inclusion of the MIT license mean that:

  1. the entire project is still Apache 2 licensed, and this MIT license is just the permission notice for using the MIT-licensed rjsf-material-ui theme in the first place, or
  2. all code in this folder material-ui is MIT licensed, and everything else in react-jsonschema-form is Apache-2 licensed, or
  3. all code added in this PR is MIT licensed, and everything else in react-jsonschema-form (including future contributions to the material-ui folder) is Apache-2 licensed?

Just want to figure out what exactly adding this license would mean for the project.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, I'm not sure, but I think the option 2.

To do the thing easier, I'm going to delete de license and leave al project with Apache 2 license 👍

@epicfaace
Copy link
Member

epicfaace commented Sep 4, 2019

Thanks. Let's use this PR to discuss how best to integrate the material ui theme, and future themes, into rjsf. I think, first, it would be good to move "material-ui" into a "themes" folder first, so we have a clear location of where to put future themes.

Option 1 - one package

Keep the material ui theme within react-jsonschema-form. Then, to use the material ui form, it would involve:

import Form from "react-jsonschema-form/themes/material-ui";

Benefits:

  • Simpler to manage/release one package rather than multiple packages

Drawbacks:

  • One package may get very large; you shouldn't need to download all themes just to use rjsf.

Option 2 - multiple packages

Publish @rjsf/material-ui as its own npm package, which has react-jsonschema-form (maybe later renamed @rjsf/core?) as a peer dependency. This is more like what this PR has now. To use the form, one would do:

import Form from "@rjsf/material-ui";

We would use fixed/locked mode with lerna to publish packages.

Benefits:

  • Package size is smaller
  • If our official themes are done this way in separate packages, it makes it easier for others to create non-official themes and still import them the same way.

Drawbacks:

  • We would need to be more careful about backwards compatibility. Each theme (such as @rjsf/material-ui) would have to declare the right version of rjsf in its peer dependencies. Right now, the widgets API
  • Need to manage multiple packages (but maybe this won't be that bad with lerna)
  • To use a theme, one would have to download a separate npm package (but this could also be a benefit, since, as discussed above, it would make the way for themes to integrate with rjsf consistent with non-official themes)

I'm leaning towards Option 2 (what we currently have in the PR) here, but just wanted to lay out my thoughts so we thoroughly consider both options. What do you think -- anything I missed?

@@ -0,0 +1,15 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Also, do we need the examples directory? Could we potentially merge this with the playground?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, of course. This isn't necessary when we have separate the core between themes.

@epicfaace epicfaace mentioned this pull request Sep 9, 2019
@agustin107
Copy link
Member Author

Thanks. Let's use this PR to discuss how best to integrate the material ui theme, and future themes, into rjsf. I think, first, it would be good to move "material-ui" into a "themes" folder first, so we have a clear location of where to put future themes.

Option 1 - one package

Keep the material ui theme within react-jsonschema-form. Then, to use the material ui form, it would involve:

import Form from "react-jsonschema-form/themes/material-ui";

Benefits:

  • Simpler to manage/release one package rather than multiple packages

Drawbacks:

  • One package may get very large; you shouldn't need to download all themes just to use rjsf.

Option 2 - multiple packages

Publish @rjsf/material-ui as its own npm package, which has react-jsonschema-form (maybe later renamed @rjsf/core?) as a peer dependency. This is more like what this PR has now. To use the form, one would do:

import Form from "@rjsf/material-ui";

We would use fixed/locked mode with lerna to publish packages.

Benefits:

  • Package size is smaller
  • If our official themes are done this way in separate packages, it makes it easier for others to create non-official themes and still import them the same way.

Drawbacks:

  • We would need to be more careful about backwards compatibility. Each theme (such as @rjsf/material-ui) would have to declare the right version of rjsf in its peer dependencies. Right now, the widgets API
  • Need to manage multiple packages (but maybe this won't be that bad with lerna)
  • To use a theme, one would have to download a separate npm package (but this could also be a benefit, since, as discussed above, it would make the way for themes to integrate with rjsf consistent with non-official themes)

I'm leaning towards Option 2 (what we currently have in the PR) here, but just wanted to lay out my thoughts so we thoroughly consider both options. What do you think -- anything I missed?

So sorry for the delay with this response, but I didn't time to see this.
By the way, I totally agree with you, I consider that option 2 is the best option, leaving free the user to add as dependency only the theme that they wanna use and keep separate the core. :)
For this, what do you need of me to move forward with this PR?

@epicfaace
Copy link
Member

@agustin107 can you move the theme into a themes folder for now? I think then we can merge and then take care of other issues after this initial PR is merged.

@agustin107
Copy link
Member Author

@agustin107 can you move the theme into a themes folder for now? I think then we can merge and then take care of other issues after this initial PR is merged.

Great! Yes of course.

@epicfaace
Copy link
Member

Thanks!

@epicfaace epicfaace merged commit 761ef57 into rjsf-team:master Sep 20, 2019
@agentmilindu
Copy link

@agustin107 @epicfaace This is wonderful! How we can make use of this?

import { withTheme } from 'react-jsonschema-form';
import { Theme as MuiTheme } from 'rjsf-material-ui';

or

import Form from "react-jsonschema-form/themes/material-ui";

or

import { withTheme } from "react-jsonschema-form/core";
import MUITheme from "react-jsonschema-form/themes/material-ui";

@epicfaace
Copy link
Member

@agentmilindu I'm working on setting this up in #1641

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

Successfully merging this pull request may close these issues.

3 participants