Skip to content
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

Fix graph issues #178

Merged
merged 6 commits into from Mar 28, 2018

Conversation

Projects
None yet
3 participants
@chriddyp
Copy link
Member

commented Mar 27, 2018

  • should fix plotly/dash-docs#81
  • also adds a resize after first plot in case graphs aren’t sized up in their container

chriddyp added some commits Mar 27, 2018

don't use `state` for `hasPlotted`
`state.hasPlotted` isn’t used in any rendering, so we don’t need to
trigger the react lifecycle through `setState`
only bind events on first plot
instead of clearing the listeners before plotting. clearing the
listeners was causing some problem for some reason, not sure why.
@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2018

@mjkramer - If you're still around, would you mind elaborating on this comment? #171 (comment) I'm essentially patching this with that suggestion but I'm forgetting what you meant by

OK, previous "solution" is no bueno because it prevents the handlers from seeing the updated props. Now I see why bindEvents was being called repeatedly.

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2018

@plotly/dash or @plotly/frontend - could someone review?

chriddyp added a commit to plotly/dash-docs that referenced this pull request Mar 27, 2018

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2018

@charleyferrari - could you check if this fixes the resizing issue that you saw in your latest dashboard project? try out the prerelease with

pip install dash-core-components==0.21.1rc4
@mjkramer

This comment has been minimized.

Copy link

commented Mar 27, 2018

Hi Chris. When bindEvents is only called once, don't those callbacks end up always seeing the props as they were at the time that bindEvents was called? Or is it irrelevant (i.e. do clear_on_hover, setProps, and fireEvent never change)? Why not, in addition to the if (!this._hasPlotted) fix, get rid of the props argument to bindEvents, and just let the callbacks access this.props directly? I do this in my components, and while I can't claim to fully understand what's going on under the hood, I haven't seen any issues.

chriddyp added some commits Mar 27, 2018

remove check for rebinding events
`setProps` and `fireEvent` are the connected functions that
`dash-renderer` passes into the components to propagate changes from
the component to the greater dash application.

these props should either be supplied all of the time or none of the
time, as the component dependency tree is defined up-front in the app.
@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2018

thanks @mjkramer ! agreed, this.props should suffice here.

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2018

Fixes the issues in the docs plotly/dash-docs#81
image

@nicolaskruchten

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

The resize thing looks good to me!

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2018

@plotly plotly deleted a comment from charleyferrari Mar 28, 2018

@chriddyp chriddyp merged commit 81dfc3b into master Mar 28, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review approved by Chris Parmer
Details

@chriddyp chriddyp deleted the fix-graphs branch Mar 28, 2018

chriddyp added a commit to plotly/dash-docs that referenced this pull request Mar 29, 2018

try out plotly/dash-core-components#178 (#85)
* try out plotly/dash-core-components#178

* Update requirements.txt

* 0.21.1rc4

* dcc v0.21.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.