Skip to content

Conversation

@BeastyBlacksmith
Copy link
Contributor

@BeastyBlacksmith BeastyBlacksmith commented Jul 28, 2023

Fixes #3
Closes plotly/Dash.jl#214

@etpinard
Copy link
Contributor

thanks for taking this on @BeastyBlacksmith !!

It would be nice a add a test before merging.

@etpinard
Copy link
Contributor

etpinard commented Aug 1, 2023

Related: plotly/Dash.jl#221

Comment on lines +7 to +8
pl = @test_nowarn DashBase.to_dash(plot(1:5))
@test pl isa PlotlyJS.SyncPlot
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard do these need to return something readable by JSON3 or is it enough, that they return something that has a to_dash method?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. Have you tried it inside a Dash.jl callback?

@etpinard etpinard mentioned this pull request Aug 2, 2023

import DashBase
import PlotlyBase
import PlotlyBase.JSON
Copy link
Contributor

Choose a reason for hiding this comment

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

This would of course break if PlotlyBase stops importing JSON.

I think this here is fine for now, but we should probably make the switch to using JSON3 exclusively inside DashBase.jl and Dash.jl. We should open an issue once this PR is merged.

Looks like PlotlyBase does define JSON3-compatible StructTypes https://github.com/JuliaPlots/PlotlyBase.jl/blob/master/src/json3.jl, but I haven't tested it

@etpinard etpinard added this to the v1 milestone Aug 3, 2023
@etpinard
Copy link
Contributor

etpinard commented Aug 3, 2023

Ok. I'm going to go ahead and merge this PR.

The other things I'd like to include in v1 are in https://github.com/plotly/DashBase.jl/milestone/1

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.

Support figures generated by Plots.jl with a PlotlyBackend Add CI workflow

2 participants