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

Enable antd (ant-design) support #1561

Merged
merged 39 commits into from
Jun 23, 2020
Merged

Enable antd (ant-design) support #1561

merged 39 commits into from
Jun 23, 2020

Conversation

delyanr
Copy link
Contributor

@delyanr delyanr commented Dec 29, 2019

Reasons for making this change

Implement antd (ant-design) theme, fields and widgets as separate sub-package. Usage is as simple as importing the Form component from the package.

Pending to-dos:

Regarding the last to-do, could you let me know what needs to be done to publish the package (I've never used lerna before)?

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'm adding a new feature
    • I've updated the playground with an example use of the feature

@delyanr
Copy link
Contributor Author

delyanr commented Jan 8, 2020

Hi. I've added a README.md file. I believe this can already be merged without the playground and especially without the withTheme dependencies, so people can start testing it. Once these dependent pull requests are merged, I can update my code as well. @epicfaace, let me know what you think?

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! This will be a great addition to the library.

I'm going to focus on getting the playground (#1539) done before merging additional themes, so it will be easier to test and preview what's going on.

"less-loader": "^5.0.0",
"react": "^16.8.6",
"react-dom": "^16.8.6",
"react-jsonschema-form": "^2.0.0-alpha.1",
Copy link
Member

Choose a reason for hiding this comment

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

I think you can move some of these (react, react-dom, react-jsonschema-form) to devDependencies.

@@ -0,0 +1,79 @@
{
"name": "@react-jsonschema-form/antd",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "@react-jsonschema-form/antd",
"name": "@rjsf/antd",

This is the name under which the package will be published. Can you rename to the above? Once this PR is merged, I can publish it under the rjsf npm organization.

Copy link
Member

Choose a reason for hiding this comment

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

Also please replace other occurrences in the readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to double-check, will the core package remain react-jsonschema-form, or will that also migrate to the rjsf npm organization?

Copy link
Member

Choose a reason for hiding this comment

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

I'm leaning more towards keeping it called react-jsonschema-form. It would be easier to upgrade that way; and that way, if someone installs the @latest version of it, they won't get the v2.0.0-alpha.1 alpha version.

Copy link
Member

Choose a reason for hiding this comment

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

Hey, just to let you know, I've updated this -- changed the name back to @rjsf/core for the core package -- it seemed best to keep all rjsf packages under the @rjsf organization to maintain a clear separation between org-owned packages and other community packages. We can still keep this called @rjsf/antd though.


### Prerequisites

- `antd == 3` (not tested yet with version 4)
Copy link
Member

Choose a reason for hiding this comment

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

Do you envision having to make a separate theme to support antd 4, or can one theme support both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, antd 4 introduced breaking changes related to the Form component. I haven't looked into the details yet, but I think we need to fix a period of time where we support two themes, and then slowly deprecate antd 3.

- `react-jsonschema-form >= 1.6.0`

```sh
npm install antd @react-jsonschema-form/core
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
npm install antd @react-jsonschema-form/core
npm install antd react-jsonschema-form

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto

@jeremypeters
Copy link

Hi. I'm using Ant Design and would like to use react-jsonschema-form. I need to settle on an approach very shortly, and I was just wondering if there is a rough ETA for this merge?

@delyanr
Copy link
Contributor Author

delyanr commented Mar 1, 2020

@jeremypeters Just saw that the dependencies from OP are now merged. So I will have a look and finalise this pull request in the next week or so. I'm very busy at the moment, unfortunately, but will get to it.

@jeremypeters
Copy link

Thanks @delyanr . No worries at all - totally understand. I'll just use the Ant Design CSS for the time-being 👍

@delyanr
Copy link
Contributor Author

delyanr commented Mar 10, 2020

@epicfaace Almost at the finish line now with this pull request. Seems like the checks are failing because the @rjsf/antd must be published first; otherwise the playground cannot find the dependency. Is that something you can do?

Finally, I spent a decent amount of time getting the antd styles to work in the playground but failed miserably. Any help to get that one, so we can finally merge would be greatly appreciated. Any ideas why that might not be working correctly? Thanks!

@erikhofer
Copy link

Hey! Any update on this? :)

@wenerme
Copy link

wenerme commented Apr 15, 2020

Base on previous work, I created a workable antd 4.0 with 2.0.0 rjsf, storybook here https://apis.wener.me/storybook/?path=/story/rjsf-demo--playground ,not pub to npm yet, consider to make a pr, but for now, anyone want to use antd, just copy and paste https://github.com/wenerme/apis/tree/master/packages/ui/stories/rjsf/antd. then, everything should just works!

@delyanr
Copy link
Contributor Author

delyanr commented Apr 15, 2020

@erikhofer I'm going through an internal update to v4 this week, so will have a look at this during the weekend to try and finalise it (and maybe split into v3 and v4 versions). In the meantime, what @wenerme pointed to should work. Thanks

@wenerme
Copy link

wenerme commented Apr 15, 2020

Current antd rjsf is followed the bootstrap style, but should not follow every style, antd's component can be more expressive,like the title and description, reorder should also support dnd, can support more input widgets.

@epicfaace
Copy link
Member

@delyanr thanks for your help! Now that we have the infrastructure for tests / etc. for material-ui ready, we should be able to merge this in soon.

Could you add some simple snapshot tests such as the ones in material-ui (https://github.com/rjsf-team/react-jsonschema-form/tree/master/packages/material-ui/test)?

<Select.Option
disabled={enumDisabled && enumDisabled.indexOf(value) !== -1}
key={String(optionValue)}
value={String(optionValue)}
Copy link

Choose a reason for hiding this comment

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

shouldn't this just be value={optionValue}? OptionValue can be a number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this a while ago now, but I remember I had some issues with boolean values. Playing around with the latest Antd version, I see that they work well, but are not part of the typescript props, so prefer to still pass through strings to be honest.

getPopupContainer={getPopupContainer}
id={id}
mode={typeof multiple !== 'undefined' ? 'multiple' : undefined}
name={id}
Copy link

Choose a reason for hiding this comment

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

I don't think name is one of the Antd Select props -- any reason this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There have been issues raised around the name attribute for the Select component, e.g. ant-design/ant-design#12612. Generally agree that this should be part of the Select component. However, for the purposes of RJSF we don't need it, so happy to remove. Any reason we should not leave it though?

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.

This looks really good -- thanks for your work on this. Can you address the license issue -- and then we can merge?

Some things which we should fix (but need not block this merge):

  • When I update the value of altdatewidget / alttimewidget, it doesn't call onChange properly -- the formData isn't updating on the playground.
  • When I change the name of the key in additionalProperties and focus out, the key name changes, but then the textbox for the key becomes blank (when it should actually still have the same value as before).
  • It would be nice to (later) convert this source to typescript and use tsdx instead of webpack for building, so we have some kind of unified build process for all themes (the hope is that we can do this with @rjsf/core eventually, too).
  • If someone has already imported the antd css in their project, is there a way to still use @rjsf/antd without causing duplicate imports (as it stands, I believe we will end up having this css duplicated)?

packages/antd/package.json Outdated Show resolved Hide resolved
@delyanr delyanr requested a review from agustin107 as a code owner June 14, 2020 18:45
@delyanr
Copy link
Contributor Author

delyanr commented Jun 14, 2020

This looks really good -- thanks for your work on this. Can you address the license issue -- and then we can merge?

Some things which we should fix (but need not block this merge):

  • When I update the value of altdatewidget / alttimewidget, it doesn't call onChange properly -- the formData isn't updating on the playground.
  • When I change the name of the key in additionalProperties and focus out, the key name changes, but then the textbox for the key becomes blank (when it should actually still have the same value as before).
  • It would be nice to (later) convert this source to typescript and use tsdx instead of webpack for building, so we have some kind of unified build process for all themes (the hope is that we can do this with @rjsf/core eventually, too).
  • If someone has already imported the antd css in their project, is there a way to still use @rjsf/antd without causing duplicate imports (as it stands, I believe we will end up having this css duplicated)?

I've fixed the first two points. Not sure I have the time to address the other two at this time, sorry. Once this is merged, others are free to contribute, etc. Thanks

@@ -64,8 +64,6 @@ const SelectWidget = ({

const { enumOptions, enumDisabled } = options;

const emptyValue = multiple ? [] : '';
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? I think this is required for multiple select support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so too, but seems like Ant Design component handles it just fine. Having the emptyValue however breaks the placeholder, so that's a bug there, and hence why I removed it. You can try it in the playground.

@epicfaace
Copy link
Member

For some reason the deploy preview (https://deploy-preview-1561--rjsf.netlify.app/) is not working. Looking into it...

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.

@delyanr can you explain what antd-dayjs-webpack-plugin is for and why it's a peer dependency? If someone uses another non-webpack build system, they won't need it, right?

@epicfaace
Copy link
Member

epicfaace commented Jun 22, 2020

@delyanr is there an alternative way that lets us not include the index.less file? This is because index.less already includes the default antd theme -- how would a user use a custom theme in this case? Is there an easy way to do this when the less file is already included?

Additionally, adding index.less seems to make the entire playground automatically have the antd style applied to it:

image

versus regular playground:

image

@epicfaace
Copy link
Member

epicfaace commented Jun 22, 2020

Looks like we may need to do this for the above problem: https://ant.design/docs/react/customize-theme#How-to-avoid-modifying-global-styles -- or maybe we could try lazy-loading antd into the playground iframe somehow

@epicfaace epicfaace merged commit 959f9b3 into rjsf-team:master Jun 23, 2020
anikethsaha pushed a commit to MLH-Fellowship/react-jsonschema-form that referenced this pull request Jul 21, 2020
* Enable antd (ant-design) support

* Change name of package in local package.json for consistency

* Add local readme file

* Tidy the package dependencies

* Change README.md references to rjsf npm organisation

* Update package.json an dimports to reflect the new rjsf organization

* Deprecate antd custom withTheme as core now has forwardRef

* Ensure Theme is not default export for consistency

* Add scripts for building dist and lib in package.json

* Fix test to use non-default import

* Add babel-plugin-import to ensure styles are imported on demand

* Add rjsf-antd theme to the playground

* Migrate rjsf-antd code to antd v4 support

* Implement EmailWidget and URLWidget for rjsf-antd

* Reimplement rjsf-antd tests based on latest

* Enable rjsf-antd support in the playground

* Finalise rjsf-antd packages files

* Remove useless build:lib script

* Enable support for additionalProperties schemas

* Update the test snapshots

* Fix minor styling issues with descriptions

* Ensure popups are rendered next to fields (even when used with iframes)

* Implement AltDate/AltDateTimeWidgets for antd theme

* update package-lock

* Fix SelectWidget placeholder never being shown

* Fix issue with the alternative date pickers

* Fix issue with additionalProperties blanking out

* Change license for consistency

* Add semicolon

* build cjs, es, and umd

* antd - switch to new build process, fix global styles

Co-authored-by: Ashwin Ramaswami <[email protected]>
nickgros added a commit to nickgros/react-jsonschema-form that referenced this pull request Apr 22, 2023
- Ensure the root field is always wrapped in Form.Item

The original ternary expression has existed since the antd theme was added in rjsf-team#1561. I am not familiar enough with antd to know what, if any, undesirable effects this change could cause.
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.

6 participants