Skip to content

Conversation

crumblix
Copy link
Collaborator

@crumblix crumblix commented Nov 24, 2016

Reasons for making this change

To allow easier and more flexible integration into an existing product with a existing object structure.
I coopted the existing variable enumOptions and exposed it in the schema. I figured if it was a good enough name internally it's my best bet externally too! But I'm terrible at naming we can call it whatever you want if you like the fix.

For example:

  enum: [1, 2, 3],
  enumNames: ["one", "two", "three"]

Will render the same as:

    enumOptions: [{myvalue: 1, , mylabel: "one"}, {myvalue: 2, mylabel: "two"}, {myvalue: 3, mylabel: "three"}],
    enumLabel: "mylabel",
    enumValue: "myvalue"

Hopefully the advantages are obvious. Instead of having to copy another array I can reference directly into an existing schema. In my case a GraphQL schema.

e.g.

    enumOptions: this.props.myarray,
    enumLabel: "name",
    enumValue: "id"

Which brings me to the second part of this fix. In my case, this.props.myarray is actually a graph returning asynchronously via a promise.

When the form is initially rendered the this.props.myarray is not set. This will in fact lead to enumOptions being passed into the Select, Radio or Checkboxes widget as a boolean false.

I had hoped to move the fix higher, however doing so seemed to break string fields in the playground, so I've put the check:

  if (!enumOptions) {
    enumOptions = [];
  }

In all three of those classes.

Before someone asks I had considered considered using the data and populating a new "translated" enumOptions with just two fields value and label. It would have been a slightly easier change (not that this is big).

However I decided against it for the following reasons. There is no reason why another custom control might not wish to access other data in your structure ... why limit it to two fields?

I can envisage (and will probably implement) a custom array class based on ag-grid. I could then display the entire data in the grid and have checkboxes down the left hand side still for selection. But I could only do that if the data structure is passed completely into the control. Perhaps another case could be another field for a hover tooltip over a selection.

It just seems more flexible. Very interested in general feedback. I understand this is a feature change/request rather than a bugfix and it should be considered carefully before giving green light or it may need rework. I hope though that people like the idea in general.

This (in part) addresses:
#415

Checklist

  • [x ] 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
  • [x ] I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • [x ] I'm adding a new feature
    • [x ] I've updated the playground with an example use of the feature

I've tried every enum I could find in the playground to try and break it.

Variations of:

          enum: ["foo", "bar", "fuzz", "qux"],
          enumOptions: [{value: "foo", label: "foo"}, {value: "bar", label: "bar"}, {value: "fuz", label: "fuz"}, , {value: "qux", label: "qux"}],
          enumOptions: [{value: "foo", label: "foo", altlabel: "foo2"}, {value: "bar", label: "bar", altlabel: "bar2"}, {value: "fuz", label: "fuz", altlabel: "fuz2"}, , {value: "qux", label: "qux", altlabel: "qux2"}],
          enumLabel: "altlabel"
          enumValue: "value"

And:

          enumOptions: [{value2: "foo", label: "foo", altlabel: "foo2"}, {value2: "bar", label: "bar", altlabel: "bar2"}, {value2: "fuz2", label: "fuz", altlabel: "fuz2"}, , {value2: "qux2", label: "qux", altlabel: "qux2"}],
          enumLabel: "altlabel",
          enumValue: "value2"

And:

        enumOptions: [{value: 1, value2: 11, label: "111", label2: "1112"}, {value: 2, value2: 2, label: "222", label2: "2223"}, {value: 3, value2: 33, label: "333", label2: "3334"}],
        enumLabel: "label2",
        enumValue: "value2"

And:

        enumOptions: [{value: 1, value2: 11, label: "111", label2: "1112"}, {value: 2, value2: 22, label: "222", label2: "2223"}, {value: 3, value2: 33, label: "333", label2: "3334"}],
        enumLabel: "label2",
        enumValue: "value2"

@nvcken
Copy link

nvcken commented May 26, 2017

Hi @crumblix , do you twitter acc or else then may I discuss with you, I'm really interested in your tech stack react-jsonschema-form + GraphQL, just a few question

@glasserc
Copy link
Contributor

Hi @crumblix, interesting proposal, thanks for taking the time to write it up thoroughly!

There's two different issues here. One is how we can asynchronously fetch the schema, the other is how we can map that schema to RJSF formats or styles.

As for the first one, I agree with what @n1k0 wrote in #415 (comment), which is that I think it's a bad idea for enumOptions to not have a clear type or to change type midway through execution. It seems like it would be much clearer to completely fetch enumOptions before rendering the form.

As for the second issue, enum is defined by the JSON Schema specification. enumNames was a proposed addition, but JSON Schema has moved away from it. react-jsonschema-form tries to stay as close to that specification as possible, which is why we have issues like #532. Since enumOptions, enumLabel or enumValues are definitely not part of that spec, it seems like a bad move to add support for them. The alternative is for you to add post-processing logic that finds those enum lists and converts them to enum and enumNames using code like enum: values.map(option => option.myValue), enumNames: values.map(option.myLabel). Using functions here is even more flexible than a single string field lookup, because it can offer a fallback, look up different fields for different objects, or do a nested lookup (enumNames: values.map(option => option.meta.description.humanReadable)). So I'm also -1 on this aspect.

I bet this isn't what you wanted to hear :) It seems like what you're looking for is a generic form library for React, and react-jsonschema-form isn't really intended to be that. It's really meant to fit a use case where you have a JSON Schema that you're using for validation somewhere and want a form that matches that schema.

Hope this helps!

@glasserc glasserc closed this May 30, 2017
@glasserc
Copy link
Contributor

I just noticed that this was sitting here for 6 months and that I only just noticed it because of the new comment -- sorry for the long delay before responding :(

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.

3 participants