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 allOf support #1380

Merged
merged 20 commits into from
Dec 7, 2019
Merged

Add allOf support #1380

merged 20 commits into from
Dec 7, 2019

Conversation

epicfaace
Copy link
Member

@epicfaace epicfaace commented Jul 31, 2019

Reasons for making this change

Support for allOf -- fixes #52, fixes #1237, fixes #1378

Todo:

  • Write tests
  • Update docs
  • Let this PR be merged
  • Should we depend on json-schema-merge-allof or implement this logic into rjsf directly?

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

@epicfaace
Copy link
Member Author

@epicfaace
Copy link
Member Author

epicfaace commented Jul 31, 2019

Bug: When using allOf, with one of the options is an array with uniqueItems: true, a minItems set to greater than 0, with the checkbox widget, and with omitExtraData and liveOmit set to true, then the user cannot check any checkboxes.

Playground reproduction here (check the boxes "Omit extra data" and "Live omit")

@epicfaace
Copy link
Member Author

Bug: When using allOf, with one of the options is an array with uniqueItems: true, a minItems set to greater than 0, with the checkbox widget, and with omitExtraData and liveOmit set to true, then the user cannot check any checkboxes.

Playground reproduction here (check the boxes "Omit extra data" and "Live omit")

This bug was fixed in 4eccffa.

package.json Outdated
@@ -46,6 +46,7 @@
"@babel/runtime-corejs2": "^7.4.5",
"ajv": "^6.7.0",
"core-js": "^2.5.7",
"json-schema-merge-allof": "git+https://github.com/mokkabonna/json-schema-merge-allof.git#e43d0f2227476e069667358b86d0e8c91b50a777",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a git dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

It links to my PR here (mokkabonna/json-schema-merge-allof#5), which makes sure that mergeAllOf can be called with deep: false and thus only merges a top-level allOf. It's not strictly required, but I guess it's a performance improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it's not strictly required, I'll just remove it since that's better than depending on a git dependency.

@erunion
Copy link
Collaborator

erunion commented Aug 8, 2019

Aside from tests and docs, is this feature complete as-is? If not, how can I help out?

@epicfaace
Copy link
Member Author

@erunion tests, docs, and a playground example. Feel free to help out with either -- just let me know on this PR!

@erunion
Copy link
Collaborator

erunion commented Aug 15, 2019

@epicfaace I can help out with a playground example.

@epicfaace epicfaace mentioned this pull request Sep 10, 2019
3 tasks
@lorthirk
Copy link

lorthirk commented Nov 8, 2019

Hello all,

I would be really interested in having this merged. Is there anything I can do to help knowing little to nothing about React?

@epicfaace
Copy link
Member Author

Remaining todos:

@epicfaace epicfaace marked this pull request as ready for review November 9, 2019 22:58
@PaulNendick
Copy link

Hello All, this is holding up an important fix very far downstream elsewhere - is there any hope of seeing this PR resolved any time soon? Is there anything a novice can do to help? Thanks!

@edi9999
Copy link
Collaborator

edi9999 commented Nov 14, 2019

Looks good to me, we can merge if you think it is good

@PaulNendick
Copy link

Thanks everyone!

@lorthirk
Copy link

@epicfaace could you please fix the conflicts? We're so close!

@harrytalbot
Copy link
Collaborator

@epicfaace any chance of the conflicts being resolved soon?

@epicfaace epicfaace merged commit 4524376 into master Dec 7, 2019
@epicfaace epicfaace deleted the allof branch December 7, 2019 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants