-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
plotly-graph: add support for JSON exports #309
Conversation
src/component/plotly-graph/render.js
Outdated
@@ -51,7 +51,7 @@ function render (info, opts, sendToMain) { | |||
} | |||
|
|||
const imgOpts = { | |||
format: (PRINT_TO_PDF || PRINT_TO_EMF) ? 'svg' : format, | |||
format: (PRINT_TO_PDF || PRINT_TO_EMF) ? 'svg' : (format === 'json' ? 'full-json' : format), |
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.
This is getting a bit tortured... I would recommend hoisting this out into a little sequence of ifs
or a switch
or something :)
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.
Could we see a real mock that shows a trivial figure like |
Here's the output for mock Here's the output mock for a trivial figure
|
OK but are we committing the output of the mock 29 JSON into the repo and comparing it each time etc? |
At the moment, no we don't have JSON baselines (see #309 (comment)). It could be added later once we support deterministic identifiers in plotly.js. |
What is nondeterministic about the JSON for the blank figure? |
For the blank figure, it seems like it will be deterministic. Actually, the only thing that will be nondeterministic in the JSON for non-trivial figures will be the |
I'd love to see at least one full json-to-fullJson mock-to-baseline in the test suite for Orca moving forward, yes. The blank figure is a good start! |
After pinning all the mock's |
@@ -21,6 +21,22 @@ do | |||
format="${filename##*.}" | |||
|
|||
case "$format" in | |||
json) | |||
diff "$1/$filename" "$2/$filename" > "$3/$filename.diff" |
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 want to do a more JSON-aware kind of diff here, or does the JSON that comes out of Orca already have sorted keys ?
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 order seems to be deterministic so it's not strictly necessary. If that ever changes, I will add JSON-aware diffing. It would be a nice addition indeed.
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.
If it comes to that we could install and run https://www.npmjs.com/package/json-diff. I went with built-in diff
for the time being.
Looks solid! thanks for 🔒ing all that down so tight with the JSON comparisons! |
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.
💃
This PR adds support for JSON exports in plotly-graph following the release of plotly.js v1.53.0 and effectively closes #283
Baselines for the image tests had to be updated for them to pass (following changes introduced by bumping plotly.js from to
v1.47.2
tov1.53.0
)cc @nicolaskruchten @alexcjohnson @jonmmease