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.

Sign up for GitHub

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

Hide grid for hidden fields (material-ui, bootstrap-4, fluent-ui); pass hidden prop to ObjectFieldTemplate #2405

Merged
merged 12 commits into from
Jun 15, 2021
Merged

Hide grid for hidden fields (material-ui, bootstrap-4, fluent-ui); pass hidden prop to ObjectFieldTemplate #2405

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bc2d8bc
material-ui: hide grid for hidden fields
Fox32 May 27, 2021
d88df9b
Merge branch 'master' into feat/fix-hidden-material-ui
Fox32 Jun 4, 2021
e1dcec6
Merge branch 'master' into feat/fix-hidden-material-ui
Fox32 Jun 7, 2021
2a01aee
Update test snapshot to latest master
Fox32 Jun 14, 2021
42095ca
Merge branch 'master' into feat/fix-hidden-material-ui
Fox32 Jun 14, 2021
83adf05
Add comment
Fox32 Jun 14, 2021
0a69f63
Move ui:widget evaluation into core
Fox32 Jun 15, 2021
f8c79ec
Add test for hidden in antd
Fox32 Jun 15, 2021
37a74e7
Hide hidden fields in bootstrap
Fox32 Jun 15, 2021
15063e8
Improve tests
Fox32 Jun 15, 2021
a089c74
Remove whitespace for hidden in fluent-ui
Fox32 Jun 15, 2021
edab596
Update custom-templates.md
epicfaace Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/material-ui/src/FieldTemplate/FieldTemplate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const FieldTemplate = ({
classNames,
disabled,
displayLabel,
hidden,
label,
onDropPropertyClick,
onKeyChange,
Expand All @@ -26,6 +27,10 @@ const FieldTemplate = ({
rawDescription,
schema,
}: FieldTemplateProps) => {
if (hidden) {
return children;
}

return (
<WrapIfAdditional
classNames={classNames}
Expand Down
26 changes: 16 additions & 10 deletions packages/material-ui/src/ObjectFieldTemplate/ObjectFieldTemplate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ const ObjectFieldTemplate = ({
}: ObjectFieldTemplateProps) => {
const classes = useStyles();

const isHidden = (element: any): boolean =>
element.content.props.uiSchema["ui:widget"] === "hidden";

return (
<>
{(uiSchema['ui:title'] || title) && (
Expand All @@ -49,16 +52,19 @@ const ObjectFieldTemplate = ({
/>
)}
<Grid container={true} spacing={2} className={classes.root}>
{properties.map((element: any, index: number) => (
<Grid
item={true}
xs={12}
key={index}
style={{ marginBottom: '10px' }}
>
{element.content}
</Grid>
))}
{properties.map((element: any, index: number) =>
isHidden(element) ? (
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this change in ObjectFieldTemplate -- what does it do? Isn't the FieldTemplate change enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here (ObjectFieldTemplate) a <Grid> with a margin is added, which means that a hidden element would still take up space. We have to remove the <Grid> if the field is hidden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think the code needs a comment to explain this?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Instead of doing this, could we check if element.content === null and then remove the grid? I'm asking because it's currently possible to specify widgets in different ways than ui:widget (we can do {'ui:options': {'widget': ...) so it's good not to tie the implementation specifically to ui:widget.

Can you also add a comment describing why we're hiding the grid in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm element.content === null is not true in that case. There is still a field create, just a hidden one. Is there any other way I can use to support both cases without having to hard code it in that spot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in that case they would both evaluate the same condition: https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/core/src/components/fields/SchemaField.js#L307

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point.

I wonder if we could still pass the hidden prop over to ObjectFieldTemplate from @rjsf/core, though, as this would reduce code duplication with checking ui:widget. How feasible / easy do you think that change would be?

Copy link
Member

Choose a reason for hiding this comment

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

Doing this would also help out with #2175! (similar issue with bootstrap-4 theme)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had some trouble with nanoid, but I think I got it working now. Still have to postpone it till tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR is updated. I couldn't find a good place in core to test it, so I skipped that. I replaced the old implementation in antd. I also went through all themes and fixed #2175 for bootstrap-4 and fluid-ui.

element.content
) : (
<Grid
item={true}
xs={12}
key={index}
style={{ marginBottom: "10px" }}>
{element.content}
</Grid>
)
)}
{canExpand(schema, uiSchema, formData) && (
<Grid container justify='flex-end'>
<Grid item={true}>
Expand Down
12 changes: 12 additions & 0 deletions packages/material-ui/test/Form.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,18 @@ describe("single fields", () => {
.toJSON();
expect(tree).toMatchSnapshot();
});
test("hidden field", () => {
const schema: JSONSchema7 = {
type: "string",
};
const uiSchema: UiSchema = {
"ui:widget": "hidden",
};
const tree = renderer
.create(<Form schema={schema} uiSchema={uiSchema} />)
.toJSON();
expect(tree).toMatchSnapshot();
});
test("hidden label", () => {
const schema: JSONSchema7 = {
type: "string",
Expand Down
44 changes: 44 additions & 0 deletions packages/material-ui/test/__snapshots__/Form.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,50 @@ exports[`single fields format datetime 1`] = `
</form>
`;

exports[`single fields hidden field 1`] = `
<form
className="rjsf"
noValidate={false}
onSubmit={[Function]}
>
<input
id="root"
type="hidden"
value=""
/>
<div
className="MuiBox-root MuiBox-root-194"
>
<button
className="MuiButtonBase-root MuiButton-root MuiButton-contained MuiButton-containedPrimary"
disabled={false}
onBlur={[Function]}
onDragLeave={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onKeyUp={[Function]}
onMouseDown={[Function]}
onMouseLeave={[Function]}
onMouseUp={[Function]}
onTouchEnd={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
tabIndex={0}
type="submit"
>
<span
className="MuiButton-label"
>
Submit
</span>
<span
className="MuiTouchRipple-root"
/>
</button>
</div>
</form>
`;

exports[`single fields hidden label 1`] = `
<form
className="rjsf"
Expand Down