-
Notifications
You must be signed in to change notification settings - Fork 359
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
Use Memo and PureComponent where it was missing #1449
Use Memo and PureComponent where it was missing #1449
Conversation
eneufeld
commented
Jul 21, 2019
- Update functional components to use React.memo
- Update components to extends React.PureComponent
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.
Looks good! There's a re-definition of areEqual
in layout.tsx
, is this on purpose?
@@ -51,32 +54,56 @@ export const renderLayoutElements = ( | |||
)); | |||
}; | |||
|
|||
const areEqual = ( |
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.
Why is this re-defined here?
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.
yes, the types didn't match. But it would make sense to find a common type between MaterialLayoutType and JsonFormsPropTypes, or maybe extend the JsonFormsPropTypes, I can have a look at that
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.
Right, now that you mention it, I think this is also the case for the ExpandPanelRenderer
. I'd be great if we could unify all of these.
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.
I'll merge this one then, let's create a follow-up. Could you take care of that?
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.
Also this needs to be rebased before I can merge it.
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.
rebased and removed the unnecessary areEqual
* Update functional components to use React.memo * Update components to extends React.PureComponent
* extended the JsonFormsPropTypes by OwnPropsOfRenderer
969e5e6
to
ef4869f
Compare
I rebased and removed the unnecessary areEqual methods |