-
Notifications
You must be signed in to change notification settings - Fork 22
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
#2870 Add ability to pass in an array variable or template for dropdown field options #2915
Conversation
<FormGroup> | ||
<FormLabel>{props.schema.title}</FormLabel> | ||
<div className="text-muted form-text"> | ||
Dropdown options defined by a variable are not displayed in preview. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could show a disabled input here with the variable name or something to indicate in the preview which variable is wired up, like when you put a variable in one of the options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
/> | ||
|
||
<SchemaField | ||
label="Option labels" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number of items here not necessarily should be equal to the number of options, RJSF is fine with that.
<SchemaField | ||
label="Options" | ||
name={getFullFieldName("enum")} | ||
schema={{ | ||
type: "array", | ||
items: { | ||
type: "string", | ||
}, | ||
}} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the Options
field be required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This brings us to our discussion of a required array field. Technically I can leave the field blank and see a dropdown with no options.
Let's first define what a "required array field" means. Is an empty array okay?
Then let's think if we need to mark this field as required
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, because you default it to an empty array in the schema already, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
// @ts-expect-error -- enumNames is a valid property of the RJSF schema. | ||
const { enumNames: labels } = props.schema; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's odd that the schema type from the json schema library doesn't have the enumNames
field. Do you know if we're using an out of date version or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec for json schema also doesn't mention it 🤔
https://datatracker.ietf.org/doc/html/draft-handrews-json-schema-validation-01
Apparently it was a proposal at some point that was rejected from being added to the spec. I found this issue thread of people discussing it:
rjsf-team/react-jsonschema-form#532
Should we use the proposed oneOf
implementation from that thread as opposed to enumNames
? I understand this is supported by react-jsonschema-form, but what are the implications here if we deviate from the core json schema spec? Does this affect anything else like yup
validation or anything like that? Are we blocking ourselves from using other tools/libraries in the future that only support the spec? Just want to be thorough here before we make the decision to diverge from the core spec... Also, it will be annoying to handle type issues with this going forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, I'll check this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BALEHOK Ben and I confirmed that this approach works in RJSF (PR was merged in here: rjsf-team/react-jsonschema-form#581)
We should use the const
keyword form:
properties:
choiceField:
oneOf:
- const: a
title: Choice A
- const: b
title: Choice B
Yup: RJSF doesn't use Yup (the do their own validation, probably directly vs. the JSON Schema)
Considerations:
- If the form definition is currently using enum (is from the workshop), should be easy to convert on mount?
- If the existing form uses enum + enumNames, you can either convert on mount, or show the edit in workshop message
<FormGroup> | ||
<FormLabel>{props.schema.title}</FormLabel> | ||
<div className="text-muted form-text"> | ||
Dropdown options defined by a variable are not displayed in preview. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
ee5f609
to
d9ba8b3
Compare
userEvent.click(textOption); | ||
}); | ||
await selectSchemaFieldType( | ||
`${RJSF_SCHEMA_PROPERTY_NAME}.schema.properties.${fieldName}.default`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be be using joinName to correctly handle spaces/special characters in field names (unless we block them on the entry side)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is a test file, do you think it can be a problem in the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, didn't notice the file name! This is perfectly ok here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests! I noticed one spot that might have problems with some field names. But not a blocker to getting this merged in
Relates to #2870
Array of options with a variable (preview):
Array of options with a variable (rendered):
Set option labels (@options holds the indexes of the source array and its values):