Compare perf - pure components, disable ROC curve thumbnail animations #598
Conversation
yebrahim
changed the title
| const height = width * 0.65; | ||
| const isSmall = width < 600; |
There was a problem hiding this comment.
could this be handled by a prop?
There was a problem hiding this comment.
We already pass the maxDimension as a prop, my goal so far was to let viewers decide how they want to render given that one number.
There was a problem hiding this comment.
I'm fine with you leaving it, I just thought it was a bit strange to have behavior controlled by size like that, rather than directly through some flag.
There was a problem hiding this comment.
Viewers are treated differently from other components though, I planned for them to behave more like black boxes. They have enough information about what they can display given a certain size, but their parents don't have that insight.
| selectedIds, | ||
| workflowObjects, | ||
| }); | ||
| this._loadParameters(selectedIds); |
There was a problem hiding this comment.
there are probably quite a few places we can improve the app in this way. I'll keep out an eye for them in the future
There was a problem hiding this comment.
You mean consolidating setState calls?
I think the main issue isn't how many times the container component renders though, it's cascading the rendering. Components should be pure wherever they can be, so that a parent re-render does not trigger all of its subtree to re-render as well.
There was a problem hiding this comment.
That, and removing unnecessary setState callbacks that could be achieved using parameters
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…flow#598) * Added concatenate license command, fixed token api scanner * Fixed logic for concatenate_license, proper prints * Removed license as per @zhenghuiwang
Fixes #597.
The main problem here was that the PlotCard component was not pure. This was causing all plots to re-render fully whenever state changes on the compare page, which happens on collapsing/expanding sections, and on selecting/deselecting runs.
Simply extending
PureComponentdoesn't work here, since it gets an array of objects, so shallow comparison always fails. ImplementingshouldComponentUpdateto properly check fixes this.This PR also disables animations in thumbnailed ROC curve, and turns RunList and CompareTable into pure components, which reduces the number of re-renders when collapsing all compare page sections from 280+ to about 50. Some of these are still because of some react-vis axis mutations that I do not understand, but it's quite a bit more usable.
Tip: turn off whitespace when viewing this change.
/area front-end
/assign @rileyjbauer
This change is