attempt at adding value and label support for select control #1581
Conversation
|
Praveen Chavali seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Hi @chavalipraveen! Thank you very much for the contribution!
It would be great if you could show an example (schema + uischema + data) on what you'd like to see in JSON Forms. This would make it much easier to understand what you're going for than reading through the code changes. You could even outline your approach before implementing it to get early feedback.
I also left some review comments so you get some feedback for your code, however they are not as important as the discussion for the general approach.
I looked through the code and noticed the following (please correct me if I'm wrong):
- Introduction of a ui schema option
keyValueDelimiter - If present, it will be used to split a given option, e.g. a JSON schema enum into multiple parts
This approach has multiple downsides:
- The user has to modify their JSON Schema enum values to add the delimiter and label to them and also modify the ui schema to get it rendered in a nice way. The original approach suggested in Spectrum (using a
oneOfofconstvalues withtitlelabels) only requires changes on the JSON schema side. - By modifying the JSON schema enum values the data object will also need to contain
value-delimiter-labelas the enum value, otherwise it won't successfully validate. This is very unexpected. The label on a value should not affect the value itself. - Translation support is not possible with this approach without also changing the data values.
So in general I would much prefer the "oneOf of const values with title labels" approach as this only needs changes on the JSON schema side which after the transformation still describes exactly the same data object.
However I can see the use case of not being able to modify the JSON Schema (or not wanting to) and keeping the changes to the ui schema. In this case I would prefer an option named enumLabels (or something similar) which is an array of labels to be used for the enum values (same order as in the schema) or a map of enum values to enum labels (to avoid ordering problems when modifying the schema).
In summary:
I would vastly prefer an implementation based on the original approach suggested in Spectrum. Would you also be interested in implementing it?
| <MenuItem value={optionValue} key={optionValue}> | ||
| {optionValue} | ||
| keyValueLabels.map(option => ( | ||
| <MenuItem value={option.value} key={option.key}> |
There was a problem hiding this comment.
Keys are uses to uniquely identify elements in React. Therefore there is no need to customize it. Just using the value is good enough, as the value should be unique.
There was a problem hiding this comment.
Just using value without the label fails at ajv validation as it expects the value to be one of the allowed enums.
There was a problem hiding this comment.
I'm not sure we're talking about the same thing. What I meant is that the separate option.key is not needed, e.g.
| <MenuItem value={option.value} key={option.key}> | |
| <MenuItem value={option.value} key={option.value}> |
would also be fine.
| }) : | ||
| options.map(option => { | ||
| return option.split(delimiter, 2).map((parts: void[]) => { | ||
| return { key: parts[0], value: option, label: parts[1] }; |
There was a problem hiding this comment.
This needs some sort of sanity check whether there are actually two parts.
| }) : | ||
| options.map(option => { | ||
| return option.split(delimiter, 2).map((parts: void[]) => { | ||
| return { key: parts[0], value: option, label: parts[1] }; |
There was a problem hiding this comment.
I usually avoid function bodies which immediately start with a return as this just adds visual clutter.
|
Hi @sdirix Thanks for the detailed review comments. I will explore both approaches you mentioned and get back.
Regards, |
|
The oneOf-const approach is now implemented with #1591. Please let me know if you have further questions or feedback ;) |
As per the discussion in the following thread, trying to add support for having different key and label for select options (MaterialEnumControl).
https://spectrum.chat/jsonforms/general/for-enums-i-need-to-be-able-to-put-labels-for-the-enum-values~5b8b5153-033f-43a9-8f5e-bc27c37a2781
I am still working on adding the test for this. MaterialEnumControl doesn't seem to have existing tests (please correct me if I am wrong). So, I am writing tests for the base control and also the modification I am proposing.