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

Feature: Sankey Link Hover Styling #6839

Closed
wants to merge 6 commits into from

Conversation

TortoiseHam
Copy link

@TortoiseHam TortoiseHam commented Jan 11, 2024

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 override the sankey link hover style by providing their own css styling. With this PR users can build apps which include css like the following:

.sankey-link-hover {
    fill: black !important;
    fill-opacity: 1 !important;
  }

This makes it feasible to track individual instances across complex diagrams even when there are hundreds of links at once.

Translations:

No user-faces changes to translate.

Features, Bug fixes, and others:

Before opening a pull request, developer should:

  • make sure they are not on the master branch of their fork as using master for a pull request would make it difficult to fetch upstream changes.
  • fetch latest changes from upstream/master into your fork i.e. origin/master then pull origin/master from you local master.
  • then git rebase master their local dev branch off the latest master which should be sync with upstream/master at this time.
  • make sure to not git add the dist/ folder (the dist/ is updated only on version bumps).
  • make sure to commit changes to the package-lock.json file (if any new dependency required).
  • provide a title and write an overview of what the PR attempts to do with a link to the issue they are trying to address.
  • select the Allow edits from maintainers option (see this article for more details).

After opening a pull request, developer:

  • should create a new small markdown log file using the PR number e.g. 1010_fix.md or 1010_add.md inside draftlogs folder as described in this README, commit it and push.
  • should not force push (i.e. 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 fetch upstream/master and "merge" with master instead of "rebase".

// Figure out whether the user has provided their own sankey-link-hover style.
var styleExists = false;
for(var i = 0; i < document.styleSheets.length; i++) {
var rules = document.styleSheets[i].cssRules;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I get the following error in the console when start using hover:

Uncaught DOMException: Failed to read the 'cssRules' property from 'CSSStyleSheet': Cannot access rules

Copy link
Author

Choose a reason for hiding this comment

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

Hi @archmoj , thanks for having a look at this. I guess this is due to cross-origin resource sharing policy preventing your browser from looking at css sheets from different domains. I didn't encounter it personally, but I've added a try/catch block to allow the code to skip over css sheets that it's not allowed to read. Does it work now on your end?

@archmoj archmoj added feature something new community community contribution status: reviewable labels Jan 12, 2024
@alexcjohnson
Copy link
Collaborator

@TortoiseHam thanks for the PR! I think though we should implement it via trace attributes rather than CSS. Most users of this library are coming not directly from JS - primarily they're in Python but there are other clients too - so they don't readily have access to the CSS, and even if you're staying in JS we want the figure (data and layout objects) to be self-contained so it's portable.

I'd suggest adding just link.hovercolor, and having that handle both color and opacity in one. Like link.color it should be arrayOk so you can either provide a single value or a complete array.

The default here is kind of tricky - and in fact looking at it in detail I think the hover opacity is handled badly right now if you either provide your own link color (we always use opacity 0.4 on hover, regardless of whether the color you provide has greater or lesser - or the same - opacity) or you don't provide a color but the background is dark (the default opacity is 0.6 in that case, so on hover the link becomes less prominent, not more)

I'd suggest the default link.hovercolor be something like:

  • If link.color has opacity < 1, use the same color with greater opacity. Not sure if that should be +0.2 or *2 (both of which match the current default on a light background of 0.2 -> 0.4 but they'd behave differently on a dark background), or something in between - capped at 1 of course... maybe we see what looks good?
  • If link.color is fully opaque, look at paper_bgcolor like we do now, and if it's dark (light), alter link.color to be a similar color but brighter (darker)
  • Note the Lib.repeat we do now is unnecessary - it's totally fine to set a single color. But we do need to support link.color being either a single color or an array, and the default rules above should generate a matching shape.

@TortoiseHam
Copy link
Author

Hi @alexcjohnson , thanks for taking a look. I agree that link.hovercolor seems like a better long term design. Unfortunately I have almost no understanding of javascript and that design seems like it would be a significantly larger change to the existing codebase. Any chance that you or anyone else who knows what they're doing is now inspired to work on this? If not I can poke at a solution in that direction but it'll likely take quite a while.

If you're looking to change the current default behavior, one option would be taking a 180 degree hue rotation of link.color at an opacity of 1.0. Upside would be that it ought to be easily noticeable in most situations. Downside would be that it probably isn't as aesthetically pleasing for small/simple diagrams. If link.color is white or black might have to just set it as grey since I don't think hue rotation would work in that case.

@TortoiseHam
Copy link
Author

Alright, I think I've got the alternative method figured out. I'm closing this in favor of: #6864

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

Successfully merging this pull request may close these issues.

3 participants