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

Add fill gradients for scatter traces #6905

Merged
merged 10 commits into from
Mar 6, 2024

Conversation

lumip
Copy link
Contributor

@lumip lumip commented Feb 24, 2024

Summary

This PR adds a new attribute fillgradient to scatter traces
which allows the user to specify a gradient as a colorscale
and an orientation in which it will be applied. fillgradient
also has optional start and stop attributes which can be
used to define absolute start and stop points in plot coordiantes.
This allows to equip different traces with exactly matching fill gradients.
Omitting start and stop will apply the gradient between the extrema
of the fill polygon (along the fill direction).

When a legend is displayed, multiple passes are made through the
Drawing.setFillStyle function, for the legend previews and the
actual plot, during which the axes are re-scaled.
This leads to errors with gradients not computing start and stop
coordinates correctly. To work around this, setFillStyle was
modified to allow distinguishing between legend and plot passes.

Example Preview

scatter_fill_gradient_tonexty_toself

Discussion

I am currently a bit unhappy with the somewhat hacky solution to the
rescaling issue with legends. I did not quite understand all the technical
steps that are going on there, so I did not attempt a more sophisticated
solution here. Any advice on this is welcome.

The contribution of the PR is also limited to scatter traces, while gradients
have in the past been requested for other trace types as well (e.g. #5238, #1918).
There are also already existing gradient implementations e.g. for markers,
so unifying this functionality somewhere more central would probably be
beneficial. However, as this would appear to require some major intervention
in existing functionality, I did not attempt that.

I was also surprised by the many places where the fillcolor description was changed
in the newly generated schema: not only for scatter traces but apparently in many
other places as well, even though I only edited the attributes.js in traces/scatter.
I am not sure these are all correct, please advise.

Remaining todos for PR

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".

Adds a new attribute `fillgradient` to `scatter` traces
which allows the user to specify a gradient as a colorscale
and an orientation in which it will be applied. `fillgradient`
also has optional start and stop attributes which can be
used to define absolute start and stop points in plot coordiantes.
This allows to equip different traces with exactly matching fill gradients.
Omitting start and stop will apply the gradient between the extrema
of the fill polygon (along the fill direction).

When a legend is displayed, multiple passes are made through the
`Drawing.setFillStyle` function, for the legend previews and the
actual plot, during which the axes are re-scaled.
This leads to errors with gradients not computing start and stop
coordinates correctly. To work around this, `setFillStyle` was
modified to allow distinguishing between legend and plot passes.
@lumip lumip force-pushed the scatter-gradient-fills branch from 931766e to 45e2cda Compare February 24, 2024 18:22
@archmoj archmoj added feature something new community community contribution status: reviewable labels Feb 26, 2024
@archmoj
Copy link
Contributor

archmoj commented Feb 26, 2024

cc: #6850

test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
test/plot-schema.json Outdated Show resolved Hide resolved
- removed assert
- renamed new mocks and baselines
- used local variable instead of direct reference to trace property
- added trailing newlines to new mock .json files
…pe for greater consistency with marker.gradient.type
@archmoj
Copy link
Contributor

archmoj commented Mar 6, 2024

Great PR.
Thank @lumip 🥇 🏆
💃

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.

2 participants