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

Removed throwing error when there's an extraneous property in ui:order #814

Merged

Conversation

igorgubernat
Copy link
Contributor

Reasons for making this change

In current implementation if you use dynamic schema dependencies, you cannot specify dependent fields in 'ui:order' list. It's because if you list such field in 'ui:order' and it is currently not displayed according to it's dependency condition, the error is thrown: 'uiSchema order list does not contain '.

I do not see why we should throw an error instead of just filtering 'ui:order' array to contain fields that are present according to dependency condition. If you cannot order conditional fields along with unconditional, it significantly reduces usefulness of dynamic schema dependency feature.

My solution is to give user the ability to specify all properties in 'ui:order' and just sort them as needed.

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@edi9999
Copy link
Collaborator

edi9999 commented Jan 18, 2018

Can you update the test instead of deleting it ?

@igorgubernat
Copy link
Contributor Author

Added tests for orderProperties(). Thanks.

@@ -218,19 +218,6 @@ describe("ObjectField", () => {
expect(labels).eql(["baz", "bar", "qux", "foo"]);
});

it("should throw when order list contains an extraneous property", () => {
Copy link
Collaborator

@edi9999 edi9999 Jan 19, 2018

Choose a reason for hiding this comment

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

Can you keep this test please and change the expectation ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

DamianOsipiuk pushed a commit to Juniper/contrail-ui-third-party-cache that referenced this pull request Jan 30, 2018
- extranous properties in ui:order not throwing errors
rjsf-team/react-jsonschema-form#814
- exported validator instance for customization
rjsf-team/react-jsonschema-form#794
@ahardworker
Copy link

When is this getting merged @edi9999 ?

@edi9999
Copy link
Collaborator

edi9999 commented Apr 5, 2018

I personnaly have found that the shown warning is useful, especially when you make typos , for example if I write in my json-schema :

users

and in my ui-schema :

usesr.

Maybe this could be an option that you set globally, such as :

allowExtraneousUIOrder={true}

@ahardworker
Copy link

Oh good point. You just gave me an idea, would it be better to show a warning in the console instead of having the warning inside the form? This way you could easily track any mispelled properties and extraneous properties by looking at your console without breaking the form. What do you think?

@edi9999
Copy link
Collaborator

edi9999 commented Apr 5, 2018

Yes, I think it makes sense to use console.warning for that.

@glasserc
Copy link
Contributor

Yes, the warning when you use a property name that doesn't exist can save a lot of aggravation. I understand it isn't really helpful in the case of dynamic schemas with property dependencies, which is why I suggested in #823 that we could maybe loosen the check in only those cases. I'd prefer logic that does the right thing to an option that the user has to know to set. @jaminthorns commented in that issue that retrieveSchema "resolves" dependencies, so we might have to refactor things in order to get this right.

@SlashBin
Copy link
Contributor

SlashBin commented Apr 17, 2018

This would be very helpful for me as I'm starting to get into generating dynamic JSON Schemas based on various rules. I would like to not have to worry about also dynamically generating my UI Schema's ui:order lists if I don't have to. It should be simple enough to tell the UI Schema "If the JSON Schema has this field, display it here on the form".
Isn't the point of the UI Schema more of a suggestion to how to render a JSON Schema? Most things seem to gracefully degrade functionality if the UI Schema doesn't make your JSON Schema.

@SlashBin
Copy link
Contributor

Perhaps we could just have a * or some other indicator for "optional" fields in ui:order. For example:

"ui:order": [
    "first_name",
    "last_name",
    "date_of_birth",
    "is_pediatric*",
    "additional_comments*"
]

I would suggest a more regex-y? but it looks like that's common in property names, according to your examples.

@SlashBin
Copy link
Contributor

SlashBin commented Apr 18, 2018

Ran into a related issue today which is blocking our project, looking for advice.

We have two "enum" fields in a section that each have dependencies, when you select the "Other" there is a text entry (this is an extremely common pattern on the web, no?)

I have no way of telling ui:schema where to display these two "Other" sections which need to be directly below the selection.

"ui:order": [
      "description",
      "description_other",
      "frequency",
      "frequency_other",
      "duration",
      "planned_procedures",
      "additional_comments"
    ]

The above results in:
Invalid plan object field configuration:uiSchema order list contains extraneous properties 'description_other', 'frequency_other'.

Am I missing some more practical way of getting this behavior? Certainly this is pretty common, so I'm thinking maybe dependencies aren't the way to do it.

@glasserc

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 is a good change, thanks! Sorry we didn't get to merging it earlier.

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.

None yet

6 participants