Hide grid for hidden fields (material-ui, bootstrap-4, fluent-ui); pass hidden prop to ObjectFieldTemplate #2405
Conversation
Implementations follows the one that already exists for ant design. Closes #1974
|
hey @agustin107, can you take a look? |
|
politely bumping @agustin107 ;) or can other ppl review @Fox32 ? |
|
Not sure, @agustin107 is set as a codeowner, maybe @epicfaace can review it too |
| </Grid> | ||
| ))} | ||
| {properties.map((element: any, index: number) => | ||
| isHidden(element) ? ( |
There was a problem hiding this comment.
Why do we need this change in ObjectFieldTemplate -- what does it do? Isn't the FieldTemplate change enough?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Do you think the code needs a comment to explain this?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Doing this would also help out with #2175! (similar issue with bootstrap-4 theme)
There was a problem hiding this comment.
Had some trouble with nanoid, but I think I got it working now. Still have to postpone it till tomorrow.
There was a problem hiding this comment.
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.
Reasons for making this change
hiddenfields aren't hidden for material-ui as the grid around them is still displayed. This hides the grid for hidden fields.Implementations follows the one that already exists for ant design.
Before (note the space before the disabled example)
After
Closes #1974
Fixes #1683
Checklist
Deploy preview
Once CI finishes, you should be able to view a deploy preview of the playground from this PR at this link: https://deploy-preview-[PR_NUMBER]--rjsf.netlify.app/