rjsf-team / react-jsonschema-form

A React component for building Web forms from JSON Schema.
https://rjsf-team.github.io/react-jsonschema-form/
Apache License 2.0
14.39k stars 2.2k forks source link

Should we move to Bootstrap v4? #899

Closed glasserc closed 5 years ago

glasserc commented 6 years ago

Bootstrap 4 is out of alpha now...

I don't want rjsf to have to support too much. Ideally we could support either one or zero HTML/CSS frameworks "by default", and the other ones could be added by users in other packages or something. But I think the fact that Bootstrap v3 is now deprecated means the one we support "by default" shouldn't be that. On the other hand, moving unilaterally to a new Bootstrap might be a breaking change for our users. Or maybe it wouldn't be so bad, because it's just a version bump on a dependency in package.json. Thoughts?

ugogo commented 6 years ago

IMO we should get rid of Bootstrap to be fully agnostic. Maybe we can still support frameworks by creating plugins?

abecks commented 6 years ago

I've spent the last two days creating Bootstrap 4 widgets by overriding BaseInput, CheckboxWidget, CheckboxesWidget, etc. via the widgets prop on Form. It seems to work well this way, and I've been able to make it fully compatible with BS4 syntax so far.

This has been a breeze to setup. My only concern is maintaining separate widgets for each framework, but I was thinking of making a plugin repo and maintaining the BS4 widgets there. Any thoughts?

This is essentially how I'm doing it:

import BaseInput from './widgets/BaseInput'
import CheckboxWidget from './widgets/CheckboxWidget'
import CheckboxesWidget from './widgets/CheckboxesWidget'

const widgets = {
    BaseInput,
    CheckboxWidget,
    CheckboxesWidget,
}

const FormWrapper = function(props) {
    const idPrefix = snakeCase(props.schema.title)

    const newProps = {
        ...props,
        ...{
            noHtml5Validate: true,
            showErrorList: false,
            transformErrors,
            FieldTemplate,
            widgets,
            idPrefix,
        },
    }

    return <Form {...newProps} />
}
FormWrapper.propTypes = {
    schema: PropTypes.object.isRequired,
}

I then export my FormWrapper and use that instead of Form from this package.

I've created custom ErrorList, Label, Help, FieldTemplate components, as well as one for each widget. Overriding the default widget association is easy with the widgets prop (kudos!).

You can find my code here: https://github.com/abecks/react-jsonschema-form-plugin-bs4

If you think this is a good approach, I will add webpack and release it on NPM.

glasserc commented 6 years ago

This is the approach I'd like to see (refs #623, #555), but I'm not sure that what we currently have is sufficient to actually support this. Previous users who have requested this sort of feature have never explained exactly what's missing, but in particular, I think ArrayField might turn out to be tricky. If you could demonstrate that your bs4 library supported 100% of the functionality that rjsf wants to support, I would be quite excited about it! If not, please feel free to open issues/PRs to add whatever's needed.

glasserc commented 6 years ago

Also refs #832.

abecks commented 6 years ago

Hey @glasserc,

I see what you mean about ArrayField. I could continue to override the fields like the widgets, but I'm concerned that too much logic is ending up in what should just be a theme.

I think a better solution would be to allow themes to add classes and markup to the existing fields/widgets, without duplicating logic into the theme.

We could use an extensive dictionary to provide markup and classes to the components:

const bootstrapTheme = {
  widgets: {
    BaseInput: {
      className: 'form-control'
    },
  }
}

A simple example for \<input>:

// /src/components/widgets/BaseInput.js
function BaseInput(props) {
  // ...
  return (
    <input
      className={theme.widgets.BaseInput.className}  // I imagine this could be passed by FieldTemplate
  // ..
}

Classes aren't enough, and we need to be able to control surrounding markup, maybe a wrapper component:

const checkboxWrapper = props => <div className="form-check">{props.children}</div>
const bootstrapTheme = {
  widgets: {
    CheckboxWidget: {
      wrapper: CheckboxWrapper,
      className: 'form-check-input'
    },
  }
}
// /src/components/widgets/CheckboxWidget.js
function CheckboxWidget(props) {
  // ...
    return (
      <Wrapper>
        <input
      className={className} // FieldTemplate can help pass these down
          // ...
    />
      </Wrapper>
    )
}

This approach would be framework-agnostic. What do you think?

naivefun commented 6 years ago

I agree with @ugogo that this library should split from specific UI library. We do not wanna import bootstrap js/css to our app if we are building a material-ui app.

scheiblr commented 6 years ago

I think, yes :smile: I'd love to use this piece of software in a running project, but I'm on BS4. Do you have any suggestions how it's possible to run the current version and improve it to work with BS4 until the BS4 supported version is public?

Does somebody know which components are still compatible in BS4?

MatejBransky commented 6 years ago

I'm using a rewritten version of this great lib. Basically I just separated form, UI and fields from it. In an advanced mode it works like this: (it's not public)

/**
 * Form container, newly required props: fields, templates and widgets
 */
import AdvancedForm from '@react-schema-form/form';
/**
 * All fields without templates
 * Templates are moved to `templates` in a themed package like @react-schema-form/bootstrap-4
 */
import fields from '@react-schema-form/fields';
/**
 * Templates and widgets with Bootstrap 4 UI
 */
import { templates, widgets } from '@react-schema-form/bootstrap-4';

...

/**
 * Templates are stored in a registry. Registry doesn't load any default fields and widgets.
 */
<AdvancedForm fields={fields} templates={templates} widgets={widgets} {...otherProps} />

All UI elements should be updated to Bootstrap 4. Here is a quick showcase (sourcemaps enabled). For icons I'm using SVGR.

As you can see it is possible to use custom Form (I built my version with a reducer pattern so I can build dynamic form fields) and reuse fields and UI or you can build custom fields and reuse the others... It's fully customizable.

And on top of that I've created some small packages with prewired components so you can use it like this:

import Form from '@react-schema-form/form-bootstrap-4';
...
/**
 * It has prewired fields, templates and widgets 
 * so you can use it as original react-jsonschema-form.
 */
<Form fields={{ MyField }} widgets={{ MyWidget }} schema={schema} uiSchema={uiSchema} {...} />

It works great and all tests passed. 👍 (btw I rewrote all your tests to Jest + react-testing-library -> great experience, better performance and no need for Sinon). And all universal tests are in separate package so you can check compatibility in others theme-packages or custom fields packages. Example of usage:

import { initCheck } from '@react-schema-form/test-utils';
import AdvancedForm from '@react-schema-form/form';
import fields from '@react-schema-form/fields';

// custom theme
import { templates, widgets } from '../src';

const check = initCheck(AdvancedForm, { fields, templates, widgets });

check('ObjectFieldTemplate'); // tests from original ObjectFieldTemplate_test.js
check('ArrayField'); // tests from original ArrayField_test.js
...

Edit: Oh and another maybe interesting feature: I've added support for templates in a uiSchema so you can change for example FieldTemplate for one schema entity:

const uiSchema = {
  foo: { 'ui:FieldTemplate': MyFieldTemplate }
}

Once I've finished one project, I'll send a PR.

So thanks for your huge effort to build this amazing library which covers almost all of my wishes! 👍

eberkund commented 6 years ago

@MatejMazur great work, I am excited to try out your changes. Do you have the repo with your changes available on GitHub? I see a forked repo on your profile, is that it? You should be able to open a pull request from that.

MatejBransky commented 6 years ago

@eberkund I'm working on it.

Update: My solution was a part of an app so I need to update some parts.

MatejBransky commented 6 years ago

~Ok, so here is a working repo (not published on npm yet): https://github.com/react-schema-form/react-schema-form (it's still in development - the progress and changes are described at the top of the readme).~ (see conversation below)

~I moved into the new one because the solution uses different naming convention (library structure is based on a npm scope @react-schema-form).~ (see conversation below)

~I think that it deserve some special space for extensions, website, etc so I created github org github.com/react-schema-form for the whole library.~ (see conversation below)

~I don't want it to be confusing for other developers so I would like to know what to do with it next (before publishing).~

eberkund commented 6 years ago

Looks good, I am having a little trouble getting webpack to resolve the scoped packages but I will look into it more later today hopefully.

eberkund commented 6 years ago

@MatejMazur in regard to "some special space for extensions, website, etc" is the intention still to merge your changes back into the main library? I think forking it off into another library when this one already has many users would fragment resources.

MatejBransky commented 6 years ago

You're right. :+1:

leplatrem commented 6 years ago

Since v3 has a (moderate) vulnerability, we should move to v4.

richmidwinter commented 6 years ago

I'm surprised this project doesn't have bootstrap 4 support yet, it looks a little worrying for anyone looking for a healthy project to build upon.

Can the linked changes not just be put on a new major version branch where people can worry less about trivialities liked renamed variables and just see if it works? I don't see the concern over breaking changes, just give it a new major version.

leplatrem commented 6 years ago

I guess it's more a question of opening a PR indeed!

Would you be interested in looking at it @richmidwinter ?

eberkund commented 6 years ago

@MatejMazur what's the state of the work you did to migrate to v4?

@leplatrem check out: https://github.com/MatejMazur/react-schema-form

KKS1 commented 5 years ago

Is it advisable to use the other mentioned repo link for applications built on Bootstrap v4, or wait for it to me merged and fully tested here. Thanks in advance for your time!

dechamp commented 5 years ago

So what is the status of this? Is there a plan to merge in @MatejMazur or would it be better to use @abecks?

peterkelly commented 5 years ago

For those of you who do not wish to wait any longer for Bootstrap 4 support, I've ported the library and published it under a different package name on npm.

See: https://github.com/peterkelly/react-jsonschema-form-bs4

To install, just do:

npm install react-jsonschema-form-bs4

This package includes the type definitions, so there is no need for @types/react-jsonschema-form-bs4.

Version numbers for react-jsonschema-form-bs4 follow those for react-jsonschema-form and I intend to keep the former up-to-date as changes are made to the latter, until such time as it supports the latest version of bootstrap.

crobinson42 commented 5 years ago

IMO we should get rid of Bootstrap to be fully agnostic. Maybe we can still support frameworks by creating plugins?

@ugogo said it best - this should be the strategy for the future of this project! If a user wishes to use semantic-ui, bs3/4, jquery ui, etc. then they can just install the community supported ui plugin libs. I think a good project that models this is Storybook where they have tons of extendibility via plugins across both frameworks and ui's.

PhilipK2 commented 5 years ago

@peterkelly I am using your new repository but the glyphicons still arent displaying. Does anyone else have this same issue?

tomreitsma commented 5 years ago

@PhilipK2 I looked into the port commit @peterkelly made and it appears that in the index.html there's now an include of Fontawesome

<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.6.3/css/all.css">

After I added this (next to the proper bootstrap of course) it seems to be working fine.

I hope there will be a more sustainable (i.e. ui framework agnostic) solution soon.

mrbarletta commented 5 years ago

thanks for the effort @peterkelly - how hard is to update it to the latest react-jsonchema-form ? seems january was a very active month and your fork is behind.

epicfaace commented 5 years ago

Yes, having a ui framework agnostic solution would be great. (There was some stalled progress at #1013). If someone has an implementation of just the withTheme function, that would be a great feature that would solve the bootstrap 4 problem and I'd be willing to merge to the library.

FaizuddinEverest commented 5 years ago

BS4 upgrade would be wonderful. +1

nop33 commented 5 years ago

https://github.com/mozilla-services/react-jsonschema-form/issues/899#issuecomment-451849732

Give this man a cookie 🍪

peterkelly commented 5 years ago

thanks for the effort @peterkelly - how hard is to update it to the latest react-jsonchema-form ? seems january was a very active month and your fork is behind.

I've just published an update; it's now on v1.3.0

flavio-dev commented 5 years ago

same as @PhilipK2 : latest bootstrap has dropped glyphicon icon font. so the buttons for arrays are not being displayed properly.

https://getbootstrap.com/docs/4.3/migration/#components

flavio-dev commented 5 years ago

managed to make it work by importing some extra css as per answer here: https://stackoverflow.com/questions/32612690/bootstrap-4-glyphicons-migration

Also realised that the bootstrap 4 changed classes from array-item-list to list-group and array-item to list-group-item. So same stuff really, any chance to update that?

epicfaace commented 5 years ago

Closing this, let's move the discussion to https://github.com/mozilla-services/react-jsonschema-form/issues/1222