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

Hover fix for colorbars with custom tickval / ticktext #1891

Merged
merged 5 commits into from
Jul 18, 2017

Conversation

etpinard
Copy link
Contributor

fixes https://community.plot.ly/t/tooltips-dont-show-up-in-a-heatmap/4972 first reported by @empet

Another bug introduced by #1770. This time appendArrayPointValue tried to dig into a trace's colorbar.tickvals and colorbar.tickvals with a 2D pointNumber leading a slew of exceptions during hover. Note that colorbar.ticktext and colorbar.tickvals are both declared as valType: 'data_array' at the moment.

The proposed fix in 0aa87c9 isn't great, but feels the safest to my eyes.

Perhaps a better, more evolved fix would make a distinction between data_array attributes that are expected to have the same dimension as of the main data coordinates (in 2D traces, that main data coordinate is z). and other data_array attributes. To note, we might have to do this to get filter and sort working for 2D traces in general.

cc @alexcjohnson

- failing on `master` during appendArrayPointValue when
  they are less items in 'colorbar.ticktext; or 'colorbar.tickvals'
  then pointNumber[0].
- test case is for surface, but same happens for other trace types
  with 2d coords.
- don't include point values of 1d array attributes
  when pointNumber is 2d in appendArrayPointValue.
- note that 1d array attributes (e.g. `x` and `y`) can still be
  set in the `module.hoverPoints`.
@etpinard etpinard added status: reviewable bug something broken labels Jul 18, 2017
@etpinard etpinard added this to the 1.29.0 milestone Jul 18, 2017
@alexcjohnson
Copy link
Collaborator

Hmm, this will work to exclude 1D arrays from getting picked up alongside 2D arrays - so if you want to include it as a quick bug fix I'm OK with it. But there are going to be cases where the aux arrays have the same dimensionality as the main array and yet still shouldn't be included. Like colorbar.ticktext on a scatter trace using marker.color. I think your "more evolved fix" may be the only way to handle this in the long run. So, another optional schema attribute like uncorrelatedArray?

TBH I'm not sure what filter and sort even mean for 2D data... but that's a discussion for another time and you're absolutely right that if we do support this in some way it will require this "more evolved fix".

@etpinard
Copy link
Contributor Author

Like colorbar.ticktext on a scatter trace using marker.color.

Good point here. Maybe it would be best to manually skip colorbar.ticktext and colorbar.tickvals in PlotSchema.findArrayAttributes?

- as they scale independently of coordinates arrays,
  so shouldn't be filtered nor included in point event data
- do not add a schema field for this now.
  + wait until solution become clearer.
@alexcjohnson
Copy link
Collaborator

Maybe it would be best to manually skip colorbar.ticktext and colorbar.tickvals in PlotSchema.findArrayAttributes?

Good idea to handle this in PlotSchema.findArrayAttributes. But I'd much prefer to do it via the schema, so it will scale to other potential such attributes later. And reflect this in at least the docstring for findArrayAttributes, and possibly in the name - findCorrelatedArrayAttributes? At the moment I don't see any more arrays that are not correlated, except for 1D x an y arrays with a 2D z (which in principle are correlated to one or the other dimension of z but not both), though these seem to be already taken care of almost accidentally since they're part of the hover data before we even get to the generic array attributes.


expect(pt.z).toBe(3, 'z');
expect(pt.text).toBe(undefined, 'undefined text items are included');
expect('id' in pt).toBe(false, 'undefined ids items are not included');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

N.B. this 🔒 0d91074

@etpinard
Copy link
Contributor Author

etpinard commented Jul 18, 2017

But I'd much prefer to do it via the schema

We would prefer that too, but I don't want to add a schema field until we investigate these uncorrelated valType: 'data_array' attributes some more - especially within the realm of transforms. So here's aa93123. Maybe colorbar.tickvals and colorbar.ticktext shouldn't be declared valType: 'data_array' to begin with? But I guess that one is more of a workspace issue.

@etpinard
Copy link
Contributor Author

Ok. I'm merging this thing.

#1894 documents the situation and potential long-term more-robust solutions.

@etpinard etpinard merged commit 81e33ae into master Jul 18, 2017
@etpinard etpinard deleted the hover-fix-colorbars-with-custom-tickvaltext branch July 18, 2017 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants