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

Should we move to Bootstrap v4? #899

Closed
glasserc opened this issue Apr 13, 2018 · 32 comments
Closed

Should we move to Bootstrap v4? #899

glasserc opened this issue Apr 13, 2018 · 32 comments

Comments

@glasserc
Copy link
Contributor

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

ugogo commented Apr 13, 2018

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

@abecks
Copy link

abecks commented Apr 14, 2018

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

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

Also refs #832.

@abecks
Copy link

abecks commented Apr 18, 2018

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

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

I think, yes 😄
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
Copy link

MatejBransky commented May 14, 2018

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

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

MatejBransky commented Jun 1, 2018

@eberkund I'm working on it.

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

@MatejBransky
Copy link

MatejBransky commented Jun 2, 2018

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

eberkund commented Jun 2, 2018

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

eberkund commented Jun 2, 2018

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

MatejBransky commented Jun 3, 2018

You're right. 👍

@leplatrem
Copy link
Contributor

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

@richmidwinter
Copy link

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

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

Would you be interested in looking at it @richmidwinter ?

@eberkund
Copy link

eberkund commented Nov 19, 2018

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

KKS1 commented Dec 6, 2018

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

dechamp commented Dec 23, 2018

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

@peterkelly
Copy link

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.

tkurki added a commit to SignalK/signalk-server that referenced this issue Jan 8, 2019
Plugin configuration UI predates the admin ui and was stuck
on Bootstrap 3, with no apparent upgrade path because
react-jsonschema-form's patrons are not interested.

Waiting paid off: there appeared a Bootstrap 4 fork
of it, react-jsonschema-form-bs4, that hopefully will be
maintained.

rjsf-team/react-jsonschema-form#899

This commit moves the plugin configuration components under
the admin ui and removes the old ui code and references to it.
The plugin list formatting is now like the rest of the UI and
most notably the selected plugin is now in the url with
scrollTo.

react-jsonschema-form-bs4 uses a newer Font Awesome than
the Core UI that the admin ui is based on, so the quick fix here
is having both old and new Font Awesomes, leaving the
TODO of removing the old one.
tkurki added a commit to SignalK/signalk-server that referenced this issue Jan 12, 2019
Plugin configuration UI predates the admin ui and was stuck
on Bootstrap 3, with no apparent upgrade path because
react-jsonschema-form's patrons are not interested.

Waiting paid off: there appeared a Bootstrap 4 fork
of it, react-jsonschema-form-bs4, that hopefully will be
maintained.

rjsf-team/react-jsonschema-form#899

This commit moves the plugin configuration components under
the admin ui and removes the old ui code and references to it.
The plugin list formatting is now like the rest of the UI and
most notably the selected plugin is now in the url with
scrollTo.

react-jsonschema-form-bs4 uses a newer Font Awesome than
the Core UI that the admin ui is based on, so the quick fix here
is having both old and new Font Awesomes, leaving the
TODO of removing the old one.
@crobinson42
Copy link

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

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

@tomreitsma
Copy link

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

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

epicfaace commented Feb 11, 2019

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

BS4 upgrade would be wonderful. +1

@nop33
Copy link

nop33 commented Mar 19, 2019

#899 (comment)

Give this man a cookie 🍪

@peterkelly
Copy link

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

flavio-dev commented Apr 5, 2019

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

flavio-dev commented Apr 9, 2019

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

Closing this, let's move the discussion to #1222

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

No branches or pull requests