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: honor layout.hoverlabel #4687

Merged
merged 9 commits into from
Mar 26, 2020
Merged

unified hover: honor layout.hoverlabel #4687

merged 9 commits into from
Mar 26, 2020

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Mar 25, 2020

Fixes #4683

TODO

  • honor bordercolor (531fe79)
  • make sure hoverlabel.bgcolor is opaque by default (cc8ca1c)

@antoinerg
Copy link
Contributor Author

antoinerg commented Mar 25, 2020

With the changes made in this PR, a bunch of mocks are now broken in unified hover modes because they contain this in their layout:

        "legend": {
            "font": {
                "family": "", 
                "size": 0, 
                "color": ""
            }, 

This is all over our mocks and I wonder why that is the case?

EDIT: Commit 4ef8205 now takes care of the problem. hoverlabel.font.* won't inherit values from legend if they are not truthy.

@alexcjohnson

@alexcjohnson
Copy link
Collaborator

Ah yeah, that's in a lot of really old mocks... my recollection is these came from the predecessor to chart studio, which at one point had a bug that it was saving effectively fullData and fullLayout instead of data and layout. Nobody would create things like that today, and aside from unified hover they have no effect, right? So I'd say we can safely remove them from these mocks.

@antoinerg
Copy link
Contributor Author

I don't particularly like the changes I made in hoverlabel_defaults.js and I wonder whether we could solve the problem differently (we could at least make the code nicer). At the very least, the new tests show that the behavior requested by @nicolaskruchten now works.

I must say however that inheriting the bgcolor from the legend will look odd in many mocks for which the legend background is transparent.

@nicolaskruchten
Copy link
Contributor

We explicitly force the legend bgcolor to transparent in some mocks?

@antoinerg
Copy link
Contributor Author

We explicitly force the legend bgcolor to transparent in some mocks?

We do. For example:

"bgcolor": "rgba(255, 255, 255, 0)",

bgcolor: fullLayout.paper_bgcolor,
title: {text: t0, font: fullLayout.hoverlabel.font},
font: fullLayout.hoverlabel.font,
bgcolor: fullLayout.hoverlabel.bgcolor || fullLayout.paper_bgcolor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need the paper_bgcolor fallback here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If hoverlabel.bgcolor is undefined, we need a fallback. We can either specify it here or specify paper_bgcolor in mockLayoutOut in order for legendSupplyDefaults to use it as a fallback. Let me know what you prefer!

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test for this?
Should we update the descriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the background color is well tested to make sure that the color is set by layout attribute hoverlabel.bgcolor > legend.bgcolor > paper_bgcolor.

We should definitely update the description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just closing the loop based on our off-GH conversation: layout.hoverlabel.bgcolor is empty by default, so that each hoverlabel knows to take its color from its trace. It only has a value if you explicitly set one, that will become the common hover label color for all traces.

@alexcjohnson
Copy link
Collaborator

Re: transparent legend bgcolor - there are two interesting cases: legend over the margin, and legend over the plot. Both of these will happen from time to time, but I'd say we should fall back on plot_bgcolor because we know that trace symbols will be visible against that. Specifically, what if we do color.combine(legend.bgcolor, layout.plot_bgcolor), so we always get 100% opaque?

@antoinerg
Copy link
Contributor Author

antoinerg commented Mar 26, 2020

Re: transparent legend bgcolor - there are two interesting cases: legend over the margin, and legend over the plot. Both of these will happen from time to time, but I'd say we should fall back on plot_bgcolor because we know that trace symbols will be visible against that. Specifically, what if we do color.combine(legend.bgcolor, layout.plot_bgcolor), so we always get 100% opaque?

I think we should get something always opaque by default and I like the idea of using color.combine. However, I wonder if we should use layout.paper_bgcolor instead of layout.plot_bgcolor.

Using plot_bgcolor leads to this result in mock 11:
Screenshot_2020-03-26_12-52-41

I think that in this case, it should be white (ie. paper_bgcolor).

cc @alexcjohnson @nicolaskruchten

@alexcjohnson
Copy link
Collaborator

I wonder if we should use layout.paper_bgcolor instead of layout.plot_bgcolor.

Yes, looking at it again I think you're right. Also because the default for legend.bgcolor is layout.paper_bgcolor. Either way there will be unpleasant cases but paper_bgcolor probably has fewer of them.

@antoinerg
Copy link
Contributor Author

I wonder if we should use layout.paper_bgcolor instead of layout.plot_bgcolor.

Yes, looking at it again I think you're right. Also because the default for legend.bgcolor is layout.paper_bgcolor. Either way there will be unpleasant cases but paper_bgcolor probably has fewer of them.

Ok, I went with paper_bgcolor in cc8ca1c. Thanks for letting me know about color.combine! It's a nice addition.

src/core.js Outdated
require('./components/legend'),
require('./components/fx'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh that was easy 😅 A little fragile though. Can we add a comment at least, saying that fx needs to come after legend for supplyDefaults to work correctly? I wonder if there are any other couplings like 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.

Done in 0d406a0

It's a bit fragile although the hoverlable_test would fail if the order was changed!

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.

Fantastic - just one request for a comment then this is ready to go! 💃

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.

hoverlabel formatting in unified mode
4 participants