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

unified hover label #4620

Merged
merged 28 commits into from
Mar 13, 2020
Merged

unified hover label #4620

merged 28 commits into from
Mar 13, 2020

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Mar 9, 2020

Closes #4477 by introducing hover modes x unified and y unified!

Codepen:

TODO:

  • test traces with MathJax in their names (not working but see unified hover label #4620 (comment))
  • style symbol based on on (marker | line).color (ec8c373)
  • investigate problem with mock legend_horizontal_autowrap (fixed in ae5b9cc)
  • fix waterfall symbols

@antoinerg antoinerg changed the title Pr unified hoverlabel unified hover label Mar 9, 2020
@archmoj archmoj added this to the v1.53.0 milestone Mar 10, 2020
@archmoj
Copy link
Contributor

archmoj commented Mar 10, 2020

Great PR!
@antoinerg could we possibly use color of each point instead of the legend?
Here is a demo, where icons for traces with (marker | line).color arrays and colorscale currently paint in black.

@etpinard
Copy link
Contributor

etpinard commented Mar 10, 2020

Very cool!

@antoinerg have you tested x unified and y unified on graphs that have traces with MathJax symbols in their name attributes? I suspect this won't work smoothly as Fx.hover does not handle MathJax promises.

We may not need to handle unified hover labels + MathJax for the first iteration, but it would be nice to confirm

src/components/fx/hover.js Outdated Show resolved Hide resolved
src/components/legend/defaults.js Outdated Show resolved Hide resolved

if(!fullLayout._infolayer || !gd.calcdata) return;
// Check whether this is the main legend (ie. called without any opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is little hacky.
What if we pass proper options to draw legend.
In that case one may look into opts._main to figure out the mocked legend (for unified hover) from the actual legend.

test/image/mocks/hovermode_xunified.json Outdated Show resolved Hide resolved
src/plots/cartesian/layout_defaults.js Outdated Show resolved Hide resolved
src/components/fx/hover.js Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Mar 10, 2020

Hover is not showing up on this mock | possibly related to not having enough space?

@antoinerg
Copy link
Contributor Author

@antoinerg have you tested x unified and y unified on graphs that have traces with MathJax symbols in their name attributes? I suspect this won't work smoothly as Fx.hover does not handle MathJax promises.

We may not need to handle unified hover labels + MathJax for the first iteration, but it would be nice to confirm

@etpinard I just tested it and you were right that Fx.hover does not handle MathJax properly. At the moment, if the trace name has MathJax in it, it will appear as a raw string in the hover label. However, this is on par with the other hover modes so I consider it out of scope for this PR. However, I'd be curious to know how much work you think it would be to add this feature.

@etpinard
Copy link
Contributor

At the moment, if the trace name has MathJax in it, it will appear as a raw string in the hover label.

That's good.

However, this is on par with the other hover modes so I consider it out of scope for this PR

I agree 100% cc #3841

@antoinerg
Copy link
Contributor Author

antoinerg commented Mar 10, 2020

@antoinerg could we possibly use color of each point instead of the legend?
Here is a demo, where icons for traces with (marker | line).color arrays and colorscale currently paint in black.

This is addressed in ec8c373 which is a big improvement. The second, third and fourth mocks in https://codepen.io/antoinerg/pen/MWwpPpa are now much better.

@archmoj
Copy link
Contributor

archmoj commented Mar 10, 2020

@antoinerg could we possibly use color of each point instead of the legend?
Here is a demo, where icons for traces with (marker | line).color arrays and colorscale currently paint in black.

This is addressed in ec8c373

Wow! Hover on those mocks look very nice. Here is the updated demo.

@alexcjohnson
Copy link
Collaborator

Just watching @nicolaskruchten demo this and saying "I have to reload because there’s no button for unified hover" - makes me think perhaps unified vs separate hover should be a new layout attribute (unifiedhover: true?) rather than part of hovermode. That way the modebar buttons would still function to toggle compare vs closest without losing your preference of what compare mode looks like.

@@ -61,6 +61,16 @@ module.exports = {
editType: 'modebar',
description: [
'Determines the mode of hover interactions.',
'If `closest`, a single hoverlabel will appear',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put flags between two * and attributes between ` chars.
E.g. this line should be

'If *closest*, a single `hoverlabel` will appear',

@antoinerg
Copy link
Contributor Author

Just watching @nicolaskruchten demo this and saying "I have to reload because there’s no button for unified hover" - makes me think perhaps unified vs separate hover should be a new layout attribute (unifiedhover: true?) rather than part of hovermode. That way the modebar buttons would still function to toggle compare vs closest without losing your preference of what compare mode looks like.

That's an interesting idea. Either this or we add a third button? cc @nicolaskruchten

@nicolaskruchten
Copy link
Contributor

We don't want to add a new attribute because there's no real meaning to unified + closest. I also don't really want to add a new button today because the mode bar is already pretty big IMO.

Maybe in unified hover mode the two existing buttons disappear by default?

@alexcjohnson
Copy link
Collaborator

We don't want to add a new attribute because there's no real meaning to unified + closest.

By itself that's true, but it serves the purpose of saving the fact that you when you enable compare mode you want it to have unified hover.

@antoinerg
Copy link
Contributor Author

Maybe in unified hover mode the two existing buttons disappear by default?

This could be achieved in 2 lines of code.

@antoinerg
Copy link
Contributor Author

I kinda like having (x|y) unified be a hovermode as it currently is. What do you think of the idea of adding a third button down the road (for the next release for example)?

cc @archmoj @nicolaskruchten @alexcjohnson

@alexcjohnson
Copy link
Collaborator

I'd be OK with, as a short-term measure, removing the hover mode buttons in one of the unified modes. Perhaps down the line this is just one more reason to put some menus into the mode bar - one for hovermode, another for drag mode...

@antoinerg
Copy link
Contributor Author

antoinerg commented Mar 13, 2020

I'd be OK with, as a short-term measure, removing the hover mode buttons in one of the unified modes.

I remove the modebar buttons when hovermode is (x|y) unified in af4a07c

Perhaps down the line this is just one more reason to put some menus into the mode bar - one for hovermode, another for drag mode...

I can open an issue for this one if you'd like!

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Beautiful!

@antoinerg antoinerg merged commit b8b7efc into master Mar 13, 2020
@antoinerg antoinerg deleted the pr-unified-hoverlabel branch March 13, 2020 20:19
@eddy-geek
Copy link

great work. two remarks from using it (through the python api)

  • The first line containing "X" value seems to be oblivious to hovertemplate image
    Is it worth an issue or just documentation update or am I missing something?

  • With plotly express color= you get a ton of repetition by default. Example based on the first Line example from the plotly.py doc:
    image

import plotly.express as px

df = px.data.gapminder().query("continent=='Oceania'")
fig = px.line(df, x="year", y="lifeExp", color='country').update_layout(hovermode='x unified')
fig.show()

I don't see an easy fix, but better documentation on the correct hover* arguments to use would help.

@nicolaskruchten
Copy link
Contributor

Indeed, the x value here is not subject to hovertemplate ... we may add a layout.xaxis.hovertemplate for this purpose (TBD).

Regarding Plotly Express, yes, in the docs here https://plotly.com/python/hover-text-and-formatting/ we recommend forcing hovertemplate=None when using compare or unified hover with PX.

@nicolaskruchten
Copy link
Contributor

Here is the result:

image

@jcunhafonte
Copy link

@antoinerg This looks awesome. Thank you.

Screenshot 2020-04-21 at 10 27 49

When there are many traces the information gets hidden. Any hint or suggestion to have the legend with scroll or above the graph?

@alexcjohnson
Copy link
Collaborator

Thanks @jcunhafonte - moved to a new issue #4786

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unified hoverlabels
7 participants