-
-
Notifications
You must be signed in to change notification settings - Fork 139
Update when figure changes regardless of revision #126
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,23 +103,22 @@ export default function plotComponentFactory(Plotly) { | |
| UNSAFE_componentWillUpdate(nextProps) { | ||
| this.unmounting = false; | ||
|
|
||
| if (nextProps.revision !== void 0 && nextProps.revision === this.props.revision) { | ||
| // if revision is set and unchanged, do nothing | ||
| return; | ||
| } | ||
|
|
||
| // frames *always* changes identity so fall back to check length only :( | ||
| const numPrevFrames = | ||
| this.props.frames && this.props.frames.length ? this.props.frames.length : 0; | ||
| const numNextFrames = | ||
| nextProps.frames && nextProps.frames.length ? nextProps.frames.length : 0; | ||
| if ( | ||
|
|
||
| const figureChanged = !( | ||
| nextProps.layout === this.props.layout && | ||
| nextProps.data === this.props.data && | ||
| nextProps.config === this.props.config && | ||
| numNextFrames === numPrevFrames | ||
| ) { | ||
| // prevent infinite loops when component is re-rendered after onUpdate | ||
| // frames *always* changes identity so fall back to check length only :( | ||
| ); | ||
| const revisionDefined = nextProps.revision !== void 0; | ||
| const revisionChanged = nextProps.revision !== this.props.revision; | ||
|
|
||
| if (!figureChanged && (!revisionDefined || (revisionDefined && !revisionChanged))) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this condition should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or even clearer: if(revisionDefined) { if (!revisionChanged) return; } else { if (!figureChanged) return; } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooh you want clear? let revisionNotDefinedAndFigureNotChanged = !revisionDefined && !figureChanged; let revisionDefinedAndRevisionNotChanged = revisionDefined && !figureChanged; if (revisionNotDefinedAndFigureNotChanged || revisionDefinedAndRevisionNotChanged) { return; } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or may as well just keep going here let revisionNotDefinedAndFigureNotChangedOrRevisionDefinedAndRevisionNotChanged = revisionNotDefinedAndFigureNotChanged || revisionDefinedAndRevisionNotChanged; if (revisionNotDefinedAndFigureNotChangedOrRevisionDefinedAndRevisionNotChanged) { return; } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #126 (comment) is great :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I was still holding on to an earlier viewpoint. I'm 💃 on the current code actually. Sorry for allll the back and forth and thanks for continuing to push this thing in the right direction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just hard to read because it's defining the do-nothing case. It might be more readable to say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mine can be simplified to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tools showed that @nicolaskruchten's and mine are not equivalent. Upon further inspection, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, github didn't show the comments above until I refreshed, will merge... |
||
| return; | ||
| } | ||
|
|
||
|
|
||