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

Fix hover label coloring on white bgcolor #3048

Merged
merged 2 commits into from
Sep 25, 2018
Merged

Conversation

etpinard
Copy link
Contributor

fixes #2981, by doing two things:

  • contrast common hover label font color w/ bgcolor
  • do not use hoverlabel.bgcolor to determine for "name" part of hover label

cc @alexcjohnson @antoinerg

More to come in #2342, but this is probably the best we can do until then.

... and fix tests that were asserting white on white
    hover label text.
- this makes the trace name visible with
  e.g. hoverlabel.bgcolor: 'white'
@etpinard etpinard added bug something broken status: reviewable labels Sep 25, 2018
@@ -2062,7 +2062,7 @@ describe('Test hover label custom styling:', function() {
});
assertCommonLabel({
path: ['rgb(255, 255, 255)', 'rgb(68, 68, 68)'],
text: [13, 'Arial', 'rgb(255, 255, 255)']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha yeah, that would be a problem! FWIW, we'd have had a chance anyway of catching this bug when we wrote the test if it said something like

path: {bg: 'rgb(255, 255, 255)', border: 'rgb(68, 68, 68)'}, // or fill & line...
text: [13, 'Arial', 'rgb(255, 255, 255)']

So it'd be immediately obvious that the path bg matches the font color, and the font cannot be seen!
As it is path has two colors so it's unclear what they mean. text is unambiguous because all items are different types, so it's OK as an array, though still perhaps awkward as you need to know the order to write the test.

@alexcjohnson
Copy link
Collaborator

Beautiful - much clearer this way! 💃

@etpinard etpinard merged commit 85a2004 into master Sep 25, 2018
@etpinard etpinard deleted the hover-labels-on-white-bg branch September 25, 2018 17:36
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 custom color bugs
2 participants