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

Allow configuring plotly.js #106

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Jul 7, 2024

Currently PlotlyBase.jl uses plotly.js 2.3.0 (also it's the version that plotly-base-artifacts provides), which is almost two years old and does not support some nice features, e.g. shape labels.

Technically, in the current PlotlyBase.jl the plotly.js version is specified in PLOTLYJS_VERSION constant.
This PR turns into into a configurable variable, that could be read/set with PlotlyBase.plotly_version() and PlotlyBase.set_plotly_version() calls, respectively.
The PR also updates the used plotly.js version to 2.33.0.

@alyst alyst force-pushed the plotly_version_config branch from bf6aa28 to f38ab60 Compare July 7, 2024 20:09
@alyst
Copy link
Contributor Author

alyst commented Jul 19, 2024

@sglyon Could you please take a look?

@hhaensel
Copy link
Contributor

Maybe, one should also adapt the deps to produce a more modern artifact?

I just bumped the plotlyjs version in our StipplePlotly package in order to take advantage of the latest plotly features, e.g. alignment in sankey plots.

So doing the same here is certainly a good idea. Before releasing, one could bump the version to today's latest version.

@hhaensel
Copy link
Contributor

Hi @sglyon ,
I think @alyst 's PR merits your consideration. Being able to set the required plotly version is a superb feature, particularly as the first release candidate of version 3.0.0 is out.
I recommend applying this PR and changing the version to the current v2.35.2.
As already commented above, plotly.js has made some advances in the past two years that should be part of PlotlyJS.

To me the alignment of sankey plots was a major breakthrough for one of my apps, where I had to fiddle with manual alignment before - and it was never error-free.

@hhaensel
Copy link
Contributor

Also PR #107 is absolutely valid and well done from my perspective.

@sglyon sglyon merged commit d3da080 into sglyon:master Dec 12, 2024
6 checks passed
@sglyon
Copy link
Owner

sglyon commented Dec 12, 2024

Thank you both!

@alyst alyst deleted the plotly_version_config branch December 12, 2024 18:06
@disberd
Copy link
Contributor

disberd commented Dec 19, 2024

Thanks @alyst for this which is a very nice contribution!

@sglyon, I see there have been a bunch of nice PR merged recently, do you have in mind to tag a new PlotlyBase release soon so that all downstream packages can benefit from these?

@disberd disberd mentioned this pull request Dec 19, 2024
@hhaensel
Copy link
Contributor

If you do so, please also consider PR #110 ;-)
However, of all the PRs certainly the least important one.

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

Successfully merging this pull request may close these issues.

4 participants