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

Modified Graph.react.js to include restyle event #483

Merged
merged 9 commits into from Mar 18, 2019

Conversation

Projects
None yet
4 participants
@mako-npm
Copy link
Contributor

commented Mar 9, 2019

#197
https://community.plot.ly/t/interactive-callback-on-legend-click/9236/6

Made changes to Graph.react.js to add support for restyle events, restyleData is now an available component_property for graph components in Dash. Toggling on/off legend items now triggers callbacks with the associated restyleData component property.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

Nice, thanks for the PR @mako-npm! A few things we'll need to finish this:

  • Looks like the linter had a complaint, you can fix it with npm run format.
  • I'd also like to add a test for restyleData - but as we don't have any graph event data tests yet, unless you're feeling adventurous I'll be happy to do that part next week.
  • There are some percy test failures. They look like flaky tests rather than anything you did in this PR, but no time like the present to fix that! Again, I can take a look at this next week.
@lo-ise

This comment has been minimized.

Copy link

commented Mar 13, 2019

Hey folks, huge thanks @mako-npm for this PR, I could really use this feature. And huge thanks @alexcjohnson for looking into the issues. Looking forward to this getting merged.

'layout': {
'width': 700,
'height': 450
}

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 15, 2019

Contributor

I'm not sure what to say about this one... I'm tempted to consider it a bug, but I don't know what we'd do to fix it. The issue - and I still don't understand why it's triggered intermittently here, but it's a real issue users will face - is that if you have an autosized graph that's initially drawn in a display: none container, it gets the default size. It keeps that default size when the container is displayed, because no redraw is triggered. But if the window is resized at all, THEN the graph gets sized correctly.

This seems to sometimes happen on CI - see eg https://percy.io/plotly/dash-core-components/builds/1576352?utm_campaign=plotly&utm_content=dash-core-components&utm_source=github_status - so I'm just locking down the size here.

button = self.wait_for_element_by_css_selector('#button')
self.snapshot(test_name + ' -> initial')

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 15, 2019

Contributor

take the snapshot after we know the button exists

@alexcjohnson alexcjohnson requested a review from T4rk1n Mar 15, 2019

gd.on('plotly_restyle', eventData => {
const restyleData = filterEventData(gd, eventData, 'restyle');
if (!isNil(restyleData)) {
if (setProps) {

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Mar 15, 2019

Contributor

🔪 Think we can remove these checks now ?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 16, 2019

Contributor

good call. In case @Marc-Andre-Rivet is already working on removing the others I'll just do this one for now.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 18, 2019

Contributor

🔪 in 8e7806b - but as commented I only changed the new prop for now.

* Data from latest restyle event which occurs
* when the user toggles a legend item
*/
restyleData: PropTypes.object,

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Mar 15, 2019

Contributor

Can we have a shape instead?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 17, 2019

Contributor

Oh right, it’s actually a length-2 array of the args to Plotly.restyle - the first one an object of <attribute.string>: <value>, the second an integer or array of integers for the trace indices. So I’ll change it to array and describe this (yes, it’s also read-only) but it doesn’t look to me as though PropTypes allows us to do better other than via custom function, which wouldn’t do the user any good. Is that right or am I missing how to do that?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 18, 2019

Contributor

Changed to array and improved docs (including read-only on all the event data props) in 688a66f

@@ -271,6 +279,12 @@ const graphPropTypes = {
*/
relayoutData: PropTypes.object,

/**
* Data from latest restyle event which occurs
* when the user toggles a legend item

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Mar 15, 2019

Contributor

It's not obvious to me if this prop is read-only or not. From reading the code I'd say changing it has no effect, so it's a read only and should be stated.

@T4rk1n

T4rk1n approved these changes Mar 18, 2019

Copy link
Contributor

left a comment

💃 Docstrings looks great!

@alexcjohnson alexcjohnson merged commit 33118ed into plotly:master Mar 18, 2019

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Thanks @mako-npm for kicking this off! This will be part of our next release, which should come out in the next couple of days.

@mako-npm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@alexcjohnson Thanks for all the work on this, looking forward to seeing it!

@mako-npm mako-npm deleted the mako-npm:Add_restyle_prop branch Mar 20, 2019

@mako-npm mako-npm restored the mako-npm:Add_restyle_prop branch Mar 20, 2019

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.