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

Added click event on pie charts. #111

Merged
merged 3 commits into from
Dec 16, 2015
Merged

Added click event on pie charts. #111

merged 3 commits into from
Dec 16, 2015

Conversation

mdtusz
Copy link
Contributor

@mdtusz mdtusz commented Dec 14, 2015

From issue #105.

It seems like a bit of an ugly hack, but there doesn't appear to be a standardized method for the click event.

The ugly part of adding slices[0][0].__data__.trace is because for some reason, only the first slice in the each loop - the largest slice - contains the trace data.

I'd prefer to make some changes before merging this in, but I'm open to suggestions.

@@ -1276,7 +1276,7 @@ fx.click = function(gd,evt){
if(gd._hoverdata && evt && evt.target) {
gd.emit('plotly_click', {points: gd._hoverdata});
// why do we get a double event without this???
evt.stopImmediatePropagation();
if(evt.stopImmediatePropagation) evt.stopImmediatePropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

@mdtusz could you elaborate on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The event from pie/index.js does not have a stopImmediatePropagation method.

Copy link
Contributor

Choose a reason for hiding this comment

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

great thanks.

@@ -373,6 +373,12 @@ pie.plot = function(gd, cdpie) {
}
}

function handleClick (evt) {
gd._hoverdata = pt;
Copy link
Contributor

Choose a reason for hiding this comment

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

is pt a plain object or an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an object. We could wrap it in an array so that points that is returned with the plotly_click event matches scatter events, but it doesn't make sense in the context - there will only be element ever passed - the slice that's clicked on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for returning an array of length 1 for consistency, even though as you said there will always be only one pt in that array.

cc @alexcjohnson thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please, make it an array. For consistency with other types, as @etpinard says, but also with the converse operation for the same type - displaying hover data - which could still include multiple slices that you want to highlight.

@mdtusz
Copy link
Contributor Author

mdtusz commented Dec 15, 2015

@etpinard it's now returned as an array in points.

@etpinard
Copy link
Contributor

Looks good.

It might be worth spending some time trying to set-up click event tests.

@etpinard
Copy link
Contributor

Looks like there's no easy way to click events within out test framework

So 💃 + #118

mdtusz added a commit that referenced this pull request Dec 16, 2015
Added click event on pie charts.
@mdtusz mdtusz merged commit 7a7a68f into master Dec 16, 2015
@mdtusz mdtusz deleted the pie-click branch December 16, 2015 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants