-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Feature: Sankey Link Hover Styling (via link hovercolor attribute) #6864
Conversation
… hover styling easily (+6 squashed commits) Squashed commits: [9555c0c] catch exceptions when trying to read access-restricted css files, and update test case to look for css styling [635e5ee] adhere to style guide CI/CD feedback [afcbcc2] remove 'const' in favor of 'var' per CI/CD rules [5e2444b] use 'var' instead of 'let' per ci/cd guide [ba96e96] add a draftlogs entry [b8238e0] Allow end users to override the sankey link hover style by providing their own css styling
@archmoj @alexcjohnson , Any feedback on this hovercolor version of the PR? I went with a grey hover default since it's easy to explain to end user and also significantly easier to implement than dynamically computing the inverse of whatever their existing color array was. |
src/traces/sankey/defaults.js
Outdated
@@ -67,7 +67,12 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout | |||
'rgba(255, 255, 255, 0.6)' : | |||
'rgba(0, 0, 0, 0.2)'; | |||
|
|||
var defaultHoverColor = tinycolor(layout.paper_bgcolor).getLuminance() < 0.333 ? | |||
'rgba(128, 128, 128, 1.0)' : | |||
'rgba(128, 128, 128, 1.0)'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between these two values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, there isn't. I left that in case y'all wanted to have the default be different between dark mode and light mode, but if it's agreed that grey is ok regardless of bgcolor then it can be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcjohnson What would you suggest here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with grey is it won’t be visible on a grey background. But semi opaque black on a light background and semi opaque white on a dark background makes it visible in all cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have specific rgba values in mind for the two cases? I'm happy to swap in whatever defaults you think look good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I thought I had written this idea down somewhere but not seeing it now. What I'd propose is to choose the default hover color based on the actual link color (and sometimes the background), either more opaque or (if already opaque) farther from the background:
var darkBG = tinycolor(layout.paper_bgcolor).getLuminance() < 0.333;
var defaultLinkColor = darkBG ? 'rgba(255, 255, 255, 0.6)' : 'rgba(0, 0, 0, 0.2)';
// Lib.repeat is unnecessary AFAICT - provide a single color and it works fine, right?
var linkColor = coerceLink('color', defaultLinkColor);
function makeDefaultHoverColor(_linkColor) {
var tc = tinycolor(_linkColor);
if (!tc.isValid()) {
// does this happen? like can linkColor be an array of numbers or something?
return _linkColor;
}
var alpha = tc.getAlpha();
if (alpha <= 0.8) {
tc.setAlpha(alpha + 0.2);
}
else {
tc = darkBG ? tc.brighten() : tc.darken();
}
return tc.toRgbString();
}
coerceLink('hovercolor', Array.isArray(linkColor) ?
linkColor.map(makeDefaultHoverColor) :
makeDefaultHoverColor(linkColor)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, cool. I've swapped the PR to your proposed defaults
@TortoiseHam Looking good me and I'd like to include it the next plotly.js minor tomorrow. |
@archmoj , I've incorporated the suggested change to the default values. It'd be awesome to see this go out in the minor :) |
Nicely done. |
Existing Behavior:
When a user hovers over a link in a Sankey diagram, the diagram changes the style of all of the links with the same title. This is useful for tracking what path a particular thing took while it travelled through the diagram. This style change leaves the color the same, but sets the link opacity to 0.4.
Problem:
The current behavior works well when there are only a small number of links, but when dealing with hundreds or thousands of links it becomes impossible to track where an individual link came from. Users have requested a solution to this several times over the past several years:
Proposed Change:
Allow end users to manually specify a link hovercolor attribute separate from the link color attribute in order to customize hover behavior. This makes it feasible to track individual instances across complex diagrams even when there are hundreds of links at once.
Translations:
I'm not sure how to go about translating the new attribute text into all of the other supported languages.
Features, Bug fixes, and others:
Before opening a pull request, developer should:
master
branch of their fork as usingmaster
for a pull request would make it difficult to fetchupstream
changes.upstream/master
into your fork i.e.origin/master
then pullorigin/master
from you localmaster
.git rebase master
their local dev branch off the latestmaster
which should be sync withupstream/master
at this time.git add
thedist/
folder (thedist/
is updated only on version bumps).package-lock.json
file (if any new dependency required).After opening a pull request, developer:
1010_fix.md
or1010_add.md
insidedraftlogs
folder as described in this README, commit it and push.git push -f
) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please fetchupstream/master
and "merge" with master instead of "rebase".