-
Notifications
You must be signed in to change notification settings - Fork 373
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
Refactor(material): Replace Hidden component #2315
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @LukasBoll , thank you for the contribution ❤️
Many of the refactoring already look good to me :) However, I discovered that you introduced new <div>
elements at some places for the removed <Hidden>
elements.
Is this really necessary? I think they are superfluous. If you just need a wrapper component to return multiple sibling elements, the React Fragment <>...</>
can be used instead. This won't create an additional DOM element.
The files with introduced div I saw are:
- packages/material-renderers/src/additional/MaterialListWithDetailRenderer.tsx
- packages/material-renderers/src/complex/MaterialAllOfRenderer.tsx
- packages/material-renderers/src/complex/MaterialAnyOfRenderer.tsx
- packages/material-renderers/src/complex/MaterialArrayControlRenderer.tsx
- packages/material-renderers/src/complex/MaterialOneOfRenderer.tsx
- packages/material-renderers/src/controls/MaterialBooleanControl.tsx
- packages/material-renderers/src/controls/MaterialBooleanToggleControl.tsx
- packages/material-renderers/src/layouts/MaterialCategorizationLayout.tsx
- packages/material-renderers/src/layouts/MaterialCategorizationStepperLayout.tsx
- packages/material-renderers/src/mui-controls/MuiAutocomplete.tsx
For the MaterialAnyOfRenderer and the MaterialArrayControlRenderer I checked whether the removed <Hidden>
resulted in a missing <div>
. This turned out to be false. Instead the newly added <div>
added in this PR was there on top. This suggests that the <div>
is not necessary.
Please check whether wrapping in a <div>
is really necessary for the aforementioned renderers. If not, use the react fragment <>
instead to return multiple sibling elements.
Removes the deprecated hidden component and instead returns null if the hidden property is set. Closes eclipsesource#2107
8dc9a70
to
e3a8a9f
Compare
Hi @lucas-koehler , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. Thanks for the updates!
Removes the deprecated hidden component and instead returns null if the hidden property is set.
Closes #2107