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 material boolean toggle control and cell #1693

Merged
merged 3 commits into from Feb 18, 2021

Conversation

lucas-koehler
Copy link
Contributor

@lucas-koehler lucas-koehler commented Feb 15, 2021

In addition to the default checkbox renderers for boolean controls and cells,
provide a control and cell rendering a boolean as a toggle.

  • Add MuiToggle wrapping a Material UI Switch
  • Add material boolean toggle control and cell that can be activated by UISchema option toggle
  • Add unit tests for control and cell
  • Add boolean toggle example to material examples showcasing a checkbox and toggle side by side.

New example:
image

In addition to the default checkbox renderers for boolean controls and cells,
provide a control and cell rendering a boolean as a toggle.

* Add MuiToggle wrapping a Material UI Switch
* Add material boolean toggle control and cell
* Add unit tests for control and cell
* Add boolean toggle example to material examples showcasing a checkbox and toggle side by side.
@coveralls
Copy link

coveralls commented Feb 15, 2021

Coverage Status

Coverage increased (+0.2%) to 88.582% when pulling bae0cc1 on material-boolean-toggle-control into bc81978 on 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.

Works great! Thanks!

I only have minor nitpicks. Also please add the toggle boolean to the control example.

} = props;
const appliedUiSchemaOptions = merge({}, config, uischema.options);
const inputProps = { autoFocus: !!appliedUiSchemaOptions.focus };
// !! causes undefined value to be converted to false, otherwise has no effect
Copy link
Member

Choose a reason for hiding this comment

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

No need for this comment

@@ -38,6 +39,8 @@ const addCustomAutocompleteControl = (examples: ReactExampleDescription[]) => {
}
return example;
});
enhancedExamples.push(booleanToggleExample);
Copy link
Member

Choose a reason for hiding this comment

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

This should be a regular example, so we don't need any changes here.

@@ -0,0 +1,66 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

The example should be a regular example in the examples package. It's fine when it doesn't work for other renderer sets. We have many of those. It's also a good way to see which renderer set supports which arguments.

Add a new example 'Boolean Toggle' that showcases checkbox vs toggle boolean rendering.
@lucas-koehler
Copy link
Contributor Author

@sdirix I made the boolean toggle a separate example and added a boolean checkbox and toggle to the control options example.

Comment on lines 140 to 144
},
checkbox: {
type: 'boolean',
description: 'Checkbox is the default control for boolean values.'
},
Copy link
Member

Choose a reason for hiding this comment

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

We already have a boolean/checkbox in the first schema. No need to re-add it here in extendedSchema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true but I'm concerned this might break the jsonforms website. There we use the base example and the one with options separately (see https://jsonforms.io/examples/control). Thus, I imagine we only use the extentedData and extendedSchema there. But I can also remove it.

Copy link
Member

@sdirix sdirix Feb 16, 2021

Choose a reason for hiding this comment

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

That's true :) I meant to remove both, the checkbox property and its Control. We don't need to show the normal checkbox in the extendedSchema and extendedUischema as it's already a part of the normal schema and uischema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, that makes sense. I'll remove it :)

@lucas-koehler
Copy link
Contributor Author

@sdirix I removed the duplicate normal checkbox from the control options example and integrated it in the original commit.

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.

LGTM!

@sdirix sdirix merged commit e2b6911 into master Feb 18, 2021
@sdirix sdirix deleted the material-boolean-toggle-control branch February 18, 2021 09:18
@sdirix sdirix added this to the 2.5.1 milestone Feb 18, 2021
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.

None yet

3 participants