Split and reuse functions reside inside index files of scattergl scatterpolargl and splom traces #3967
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
etpinard
reviewed
Jun 17, 2019
src/traces/scattergl/style.js
Outdated
|
|
||
| var DESELECTDIM = require('../../constants/interactions').DESELECTDIM; | ||
|
|
||
| function styleTextSelection(cd) { |
There was a problem hiding this comment.
Hmm, yeah this one isn't ideal. It is under a style.js, but it's not really a _module.style method. Ok if we move this thing to ./helpers.js instead?
etpinard
reviewed
Jun 17, 2019
src/traces/splom/style.js
Outdated
| var calcColorscale = require('../scatter/colorscale_calc'); | ||
| var convertMarkerStyle = require('../scattergl/convert').markerStyle; | ||
|
|
||
| function editStyle(gd, cd0) { |
There was a problem hiding this comment.
Same here, I'm not sure this belongs in a style.js file. I'd be ok we either:
- moving this logic to
splom/index.js - renaming this file to
edit_style.js
etpinard
reviewed
Jun 17, 2019
etpinard
reviewed
Jun 17, 2019
etpinard
reviewed
Jun 17, 2019
etpinard
reviewed
Jun 17, 2019
src/traces/scattergl/index.js
Outdated
| sceneUpdate: sceneUpdate, | ||
| calcHover: calcHover, | ||
| sceneUpdate: require('./scene_update'), | ||
| calcHover: hover.calcHover, |
There was a problem hiding this comment.
No need to export these off ScatterGl anymore (I think).
There was a problem hiding this comment.
Good point!
Done in bb12f68.
etpinard
reviewed
Jun 17, 2019
|
Thanks for doing this @archmoj !! I like the way you split the cross-trace plot logic into an |
|
Nicely done @archmoj !! 💃 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the
indexfiles of all traces exceptscattergl,scatterpolargl&splomare compact and only declare different functionalities of the trace.Having similar pattern in
scattegland other two can help improve the readability & requiring of different parts (e.g. when requiring parts ofscatterglinscatterpolargl).In this PR the functions inside the
indexfiles of the three are separated and moved tocalc,plot,select,hover,helpersandupdate_scenefiles.@plotly/plotly_js