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

Added an 'Omit extra data' prop to remove formData fields not represented in the schema #1283

Merged
merged 4 commits into from
Jun 22, 2019

Conversation

CodeGains
Copy link
Contributor

Reasons for making this change

Please see #1190 for details. This PR is accomplishes the same goal as #1190, but this solution does not rely on changing the DOM.

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'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

@CodeGains CodeGains mentioned this pull request May 13, 2019
@@ -1,5 +1,7 @@
import React, { Component } from "react";
import PropTypes from "prop-types";
import _pick from "lodash/pick";
Copy link
Member

Choose a reason for hiding this comment

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

Should we be calling lodash.pick and lodash.get (and adding lodash.get to package.json) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like they are both being used, but I forgot to add lodash.get to the package json when I was moving the code over from the other branch. Will update it

@@ -753,6 +753,49 @@ export function toIdSchema(
return idSchema;
}

export function toNameSchema(schema, name = "", definitions, formData = {}) {
Copy link
Member

@epicfaace epicfaace May 24, 2019

Choose a reason for hiding this comment

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

This is great -- and we can later pass on the nameSchema to all widgets if we need them to have a name. Would it be more appropriate to call it pathSchema instead though?

Additionally, given #1248, could we potentially namespace $name?

Also, in the nameSchema, how would we distinguish an array element from an actual object property called "0" or "1" or "2"?

Copy link
Contributor Author

@CodeGains CodeGains May 26, 2019

Choose a reason for hiding this comment

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

Ya, I think that's a good idea.

Also, you are right about those array indexes. I can rename them to be like "$0", "$1", "$2" ...

Copy link
Contributor Author

@CodeGains CodeGains May 31, 2019

Choose a reason for hiding this comment

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

@epicfaace got some time to work on this now, the namespace thing you guys were talking about -- that is something that doesn't exist currently, right? We could potentially namespace $name or something similar in the future.

As for the array indexes, I think even having "$0", "$1", etc. would be too common. What about something crazy for now like having escaped quotes?

For example:

deleted example because it's not a good idea

Would it be possible to namespace something like the array indexes? somePrefix + 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.

Just updated this comment because I was thinking about it I changed my mind and don't think it would be a good idea to differentiate array indexes from object properties named like "0", "1", "2", etc. because they are handled the same way when using lodash pick. In the future, if someone needed to use the name/path schema for something else and wanted to know if a number path was an array index or property, then they could check it against the schema or form data.

@epicfaace
Copy link
Member

epicfaace commented Jun 12, 2019

@CodeGains The complexity of this PR worries me (although if that complexity is needed to implement the functionality, then that's no problem). I'm trying to understand why exactly you needed to define a nameSchema -- could you explain the reason(s) you decided to go with that? I see that, unlike idSchema, nameSchema correctly handles array items that are showing up on the form -- is there anything else that it's supposed to handle?

I think we should also add tests for toNameSchema. That will also help enumerate the use cases for it.

@CodeGains
Copy link
Contributor Author

@epicfaace

Ya it got a little complex, but I tried to make it so that the changes wouldn't impact other functionality. That is the main reason why toNameSchema is its own thing, rather than trying to modify some existing thing like toIdSchema. So the complexity is all contained within this feature since nothing uses the nameSchema. As a plus, it could be nice to have for other features in the future.

You are correct about the nameSchema. It will have each array element listed separately so that dependencies in array elements are handled independently. For example:

One array element could have a dependency met and data in the corresponding dependent field. Another element might have the same dependency not met and also data in the dependent field. Data in the dependent field should only be omitted in the second element.

There's lots of tests for toNameSchema in the utils test. Do you think there should be more?

@epicfaace
Copy link
Member

Oh, sorry, forgot to expand the utils.js file (I remember seeing the tests before and wondered where it went this time haha)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants