-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Create a withTheme
higher-order component
#1222
Comments
Woah, never though I'd be mentioned here! Not sure that I'll be helpful here, but when this is implemented I'll refactor my fork to use this withTheme. What is 'MyTheme' for? Just identification? |
I already have implemented something like |
@Kenny11CZ that sounds perfect! It would be great if you could make a PR with that 😄 |
That's a good point, I don't think the first argument is needed. For reference, the original implementation of |
I would happily work to change my repo to use withTheme and submit a PR when withTheme is available in this repo. so +1 👍 to having a withTheme HOC available. |
Now that #1226 has been merged, I'm thinking about what would be the best way to add new themes going forward. Of course, we will be able to use themes from external libraries as follows:
But might it be better if we include certain themes within the library itself? i.e.,
or even something like
I'm thinking that including these themes within the library will allow for themes for a certain number of UI frameworks to be maintained and kept up to date with the latest changes in rjsf, as well as make it easier for people to use rjsf with something other than bootstrap 3 without having to look for other npm packages to find the right theme. Would be good to hear people's thoughts on this. |
@epicfaace I think that a schema like this is a good one: import { withTheme } from "react-jsonschema-form/core";
import MUITheme from "react-jsonschema-form/themes/material-ui";
const Form = withTheme(MUITheme); This way, thanks to tree shaking, in prod only the used theme remains, and you can keep all of them in one place + keep them easily updatable. (also keeps good compatibility with typescript, but that's another thing 😉 ) |
Yes, please keep additional themes as separate packages. |
@TwoAbove yes, that sounds like a good idea. |
@TwoAbove How do i use material-UI theme with react-jsonschema-form? |
@epicfaace oh oops misread the import lines as separate packages, not folders in same package (saw "core" and assumed @TwoAbove also meant separate packages -- you can use "/" in package names, right? ex: https://www.npmjs.com/package/@uifabric/icons, https://www.npmjs.com/package/@uifabric/experiments, https://www.npmjs.com/package/@material-ui/icons, https://www.npmjs.com/package/@material-ui/styles) Can still be in same monorepo if you want, just published as separate packages: https://github.com/OfficeDev/office-ui-fabric-react/tree/master/packages and https://github.com/mui-org/material-ui/tree/master/packages |
@themakerman I've created |
@agustin107 looks great! Thanks for the hard work to get the theme working. @mirajp I believe @TwoAbove might have meant a single package, since he was specifically referring to tree-shaking. I believe both approaches work though; also, another way to do it would be to use package scopes like Babel (like @babel/babel-core, do @rjsf/material-ui). So, I'm wondering now whether it would be best to have these different themes (material UI, etc.) as different repos or in subfolders of this repository a single monorepo. Having each theme in a different repo will speed up development as react-jsonschema-form will only need to change a single set of widgets -- but there might be the possibility of the theme repos falling behind and not implementing all the widget fixes (since the tests use the default theme widgets, not the custom theme widgets). On the other hand, a single monorepo will ensure that all changes/bugfixes to widgets apply to all themes, but it might involve more work when creating a widget change as it would have to be duplicated across all officially supported themes. Would appreciate any feedback / thoughts. |
@agustin107 Really nice! About the packages, that's for the maintainers to decide.
So the gist of it is whether the maintainers have the time to support different UI packages. Do they? |
@agustin107 Thanks for the hardwork. Few questions though. I actually plan to use this sometime near in production. I was worried around maintenance thoughts. So will this come under react-jsonschema-form community or will be treated as a fork and maintained on its own? @TwoAbove Would be great if we use @react-json-schema-form/material-ui and maintainers handle packages within react-jsonschema-form. If we keep it seperate there will be less community involvement and high change of project becoming stale. Anything kept under the repo react-jsonschema-form will also give devs more confidence for using it in production. Let me know your thoughts |
@themakerman We are planning to use it in production as well, @agustin107 is working on it full-time. It won't be abandoned anytime soon, don't worry 🙂 @epicfaace It's a hard decision to make, all ways have pro's and con's. EDIT: |
Thanks for the feedback. It seems better to bring in themes into the main repo, due to the advantage of keeping the themes centralized and more maintained by the community. Trying to see if we have 10 different themes, how to keep all of these themes synchronized without having to re-implement (copy-and-paste) changes 10 different times. For example, already fixes such as #1335 have not been added to cybertec-postgresql/rjsf-material-ui yet (and perhaps it's impractical to keep up!). Let's compare the DefaultNormalArrayFieldTemplate in rjsf-material-ui:
and that in rjsf:
It seems to me that the only difference here is the tag names, not the actual logic. So what if we did something like this: In RJSF:
And then rjsf-material-ui will just export a config such as this:
This way, all the theme is declaring is what is different in the theme, rather than reimplementing each field/template/widget from scratch. @LorenzHenk @agustin107 Do you think such a solution covers all the use cases for the changes you had to make when creating rjsf-material-ui? Would welcome suggestions as well. |
I am trying to integrate Ant Design, it would be great if Separate templates and allow a completely custom UI components can be merged. |
@epicfaace You are right, we are reimplementing lots of logic. |
Yes, exactly. Perhaps we should allow a theme to completely override a widget/field if there are enough changes, but in many scenarios this would let us reduce the amount of duplicated code. |
@epicfaace Hi guys, so will it be possible to have some kind of official doc section addressing how to integrate material ui with react-json-schema forms? |
@LorenzHenk @agustin107 Let's add the existing code for rjsf-material-ui to the main react-jsonschema-form repository. We can use lerna to manage packages in the monorepo and then can publish it under something like Probably eventually (perhaps in v2), the way to use react-jsonschema-form would involve installing @themakerman Probably would want to add this to the docs only once we publish this under the Thanks for all your work on this! |
Related to issue rjsf-team#1222
* Add material-ui theme Related to issue #1222 * Delete LICENSE and update README This is according to PR conversation * Move material-ui theme into themes folder and delete example folder
I'd like to assist transitioning to lerna. Is there a branch/commit/fork where lerna is being implemented? |
@jasonritchie not yet! Will post here once it's created though. |
@epicfaace It does seem like customization at deeper levels would be good. There seems to be more exploration needed to strike the right abstraction in this lib. I still think there is too much "private" logic happening that is not exposed in the right way for external customization/themeability. Which is normal; this is what happens as libraries grow organically. To fix it, it would require essentially a large rewrite or refactor. I think these are the layers of abstraction that might be needed:
|
@timkindberg thanks for your thoughts. Right now, we're going to be releasing multiple packages in v2. The initial plan was to release a package called It's worth aiming for the rewrite you mention in the long-term, and it might be best to align the package structure in v2 so that it gets closer to that structure. Right now, I'm a bit unclear as to what exactly the output of #1 would be -- what could an output of "schema-to-ui parsing" be, other than perhaps something like React components? |
Ah ok, yeah I see the output of #1 to be an agnostic JSON structure that any framework could utilize. Basically JSX in JSON form. So the html tag, the attributes, the children, plus any necessary meta information. |
Ideally, we should have a
withTheme
HOC in which fields, widgets, and/or templates can be passed -- this can be used to implement custom themes.This was implemented in #1013, but that included several other breaking changes -- so I'd like to see a PR which just implements the
withTheme
component so it can be added to the library.Example usage:
I'm listing the popular / currently maintained libraries that fork off of react-jsonschema-form and implement a UI framework other than bootstrap 3, so that the maintainers of those repos are aware of the progress on this issue -- or if anyone wants to jump in and do this, that would be great! @peterkelly @cwhatley @vip-git @SelfKeyFoundation @eddyzhang1986 @TwoAbove @nilportugues
BS4
https://github.com/peterkelly/react-jsonschema-form-bs4
Material
https://github.com/cwhatley/material-ui-react-jsonschema-form
https://github.com/vip-git/react-jsonschema-form-material-ui
https://github.com/SelfKeyFoundation/react-jsonschema-form-material-theme
https://github.com/TwoAbove/jsonschema-form-for-material-ui
Antd
https://github.com/eddyzhang1986/antd-jsonschema-form
Semantic UI
https://github.com/nilportugues/react-jsonschema-form-semanticui
See #899
The text was updated successfully, but these errors were encountered: