Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 21, 2019
Merged

Update when figure changes regardless of revision #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ In short, this means that simply adding data points to a trace in `data` or chan
| `layout` | `Object` | `undefined` | layout object (see https://plot.ly/javascript/reference/#layout) |
| `frames` | `Array` | `undefined` | list of frame objects (see https://plot.ly/javascript/reference/) |
| `config` | `Object` | `undefined` | config object (see https://plot.ly/javascript/configuration-options/) |
| `revision` | `Number` | `undefined` | When provided, causes the plot to update _only_ when the revision is incremented. |
| `revision` | `Number` | `undefined` | When provided, causes the plot to update when the revision is incremented. |
| `onInitialized` | `Function(figure, graphDiv)` | `undefined` | Callback executed after plot is initialized. See below for parameter information. |
| `onUpdate` | `Function(figure, graphDiv)` | `undefined` | Callback executed when when a plot is updated due to new data or layout, or when user interacts with a plot. See below for parameter information. |
| `onPurge` | `Function(figure, graphDiv)` | `undefined` | Callback executed when component unmounts, before `Plotly.purge` strips the `graphDiv` of all private attributes. See below for parameter information. |
Expand Down
17 changes: 8 additions & 9 deletions src/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this condition should be ( (!revisionDefined && !figureChanged) || (revisionDefined && !revisionChanged) )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or even clearer:

if(revisionDefined) {
    if (!revisionChanged) return;
}
else {
    if (!figureChanged) return;
}

Copy link
Member

Choose a reason for hiding this comment

The 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;
}

Copy link
Member

Choose a reason for hiding this comment

The 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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#126 (comment) is great :)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 if(! (<define the positive case>) )...

Copy link
Contributor Author

@dmt0 dmt0 Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mine can be simplified to
(!figureChanged || revisionDefined || revisionChanged))
Forgot all my propositional calculus, tools to the rescue :)
http://logictools.org/
(-figureChanged & (-revisionDefined | (revisionDefined & -revisionChanged)))

Copy link
Contributor Author

@dmt0 dmt0 Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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, ((!revisionDefined && !figureChanged) || (revisionDefined && !revisionChanged)) fails the same test as #126 (comment). So I'm gonna use my original. The simplified version above was my error... Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}

Expand Down