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

Add MultiEnum renderer for enums and oneOf #1709

Merged
merged 2 commits into from
Mar 10, 2021

Conversation

AlexandraBuzila
Copy link
Member

Render an anyOf array of consts as a list of checkboxes. The checkbox is
checked if the array item it represents is present in the data.

@coveralls
Copy link

coveralls commented Mar 1, 2021

Coverage Status

Coverage increased (+0.005%) to 88.587% when pulling 99fed1d on AlexandraBuzila:enum-array into 7800718 on eclipsesource:master.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

In general I like it very much. However there are some things I would like to see differently.

Currently the schema tested for looks like this

"property": {
  "type": "array",
  "items": {
    "anyOf": [
      { },
    ]
  },
},

which doesn't really fit to the use case:

  • The schema allows the same entry multiple times, but our control only handles them uniquely (the control therefore misbehaves if the same entry is there multiple times)
  • anyOf stands for that at least one item matches. However we're handling it like "enums", which are unique. So it should be oneOf.
  • To be consistent with our other oneOfEnum controls we should check whether the items are oneOf and contain a const in each entry.
  • Ideally we should also be able to handle enum content

I would prefer to support the following schemas:

"property": {
  "type": "array",
  "items": {
    "oneOf": [
      {
        "const": <any>,
        "title": "mylabel"
      },
      {
        "const": <any>,
        "title": "otherLabel"
      },
    ]
  },
  "uniqueItems": true
},

and

"property": {
  "type": "array",
  "items": {
    "type": "string",
    "enum": ["a", "b", "c"]
  },
  "uniqueItems": true
},

So we check for

  • type array with unique items
  • the items are either enum or oneOf containing const

Please also add an example to the example app.

)
);

export default MaterialEnumArrayControl;
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with other controls we should default export the wrapped version

},
{
tester: materialEnumArrayControlTester,
renderer: withJsonFormsControlProps(MaterialEnumArrayControl)
Copy link
Member

Choose a reason for hiding this comment

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

Don't wrap here, but use the already wrapped imported version

const optionPath = Paths.compose(path, `${index}`);
return (
<FormControlLabel
id={option.const}
Copy link
Member

Choose a reason for hiding this comment

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

Careful: The value of const can be anything! It doesn't need to be a string, but could for example be an object, array, etc. Please see mapStateToOneOfEnumProps where we already had to handle the case.

Comment on lines 44 to 56
const onChange = (option: string, value: any) => {
const newData = data ? [...data] : [];
if (value) {
// option was added
newData.push(option);
} else {
// option was removed
const indexInData = newData.indexOf(option);
newData.splice(indexInData, 1);
}

handleChange(path, newData);
};
Copy link
Member

@sdirix sdirix Mar 1, 2021

Choose a reason for hiding this comment

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

If you would use withJsonFormsArrayControlProps you would get functions to add and remove items to the array for free, i.e. addItem, removeItems, moveUp and moveDown would get injected. schema would also already point to the items object. Actually that doesn't make so much sense as these functions are index based, which is not really the use case here.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Additionally it would be great to extract all the logic from the renderer. I would prefer a new mapStateToMultiEnumProps and mapDispatchToMultiEnumProps combined into a React withJsonFormsMultiEnumProps.

  • mapStateToMultiEnumProps: extracting the items options from oneOf or enum, similar to mapStateToEnumPros and mapStateToOneOfEnumProps
  • mapDispatchToMultiEnumProps: providing a addItem and removeItem function which can be used for toggling. Using this we can also remove the useRef workaround

By extracting the logic from the renderer we can easily write a multi-select enum with a different renderer (e.g. Material UI's multi select dropdown) as well as more easily add the same renderer in the other renderer sets too.

@AlexandraBuzila
Copy link
Member Author

I updated the commit to include you comments, Stefan. The renderer now supports both oneOf and enum items.

Render an enum array or anyOf array of consts as a list of checkboxes.
The checkbox is checked if the array item it represents is present in
the data.
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Looks good!

export const oneOfToEnumOptionMapper = (e: any): EnumOption => ({
value: e.const,
label:
e.title || (typeof e.const === 'string' ? e.const : JSON.stringify(e.const))
Copy link
Member

Choose a reason for hiding this comment

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

We should respect empty titles ;)

Suggested change
e.title || (typeof e.const === 'string' ? e.const : JSON.stringify(e.const))
e.title ?? (typeof e.const === 'string' ? e.const : JSON.stringify(e.const))

@sdirix sdirix changed the title Add Material renderer for anyOf const array Add MultiEnum renderer for enums and oneOf Mar 10, 2021
@sdirix sdirix merged commit 85dba30 into eclipsesource:master Mar 10, 2021
@sdirix sdirix added this to the 2.5.1 milestone Mar 10, 2021
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.

3 participants