| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks again for the great comments, @emilykl ! I will attempt to reference commits inline from this comment here: -- Bug Fixes -- "The axis ranges don't automatically adjust to include the full length of the arrows" fixed here: "Colorscale seems broken in the latest version" fixed here: "Legends are missing icons" fixed here: "selected and unselected styling options" fixed here: "The text property also seems broken in the latest iteration" fixed here: "Display of colorbars does not seem to work" fixed here: -- API Refinements -- "c should become marker.color" fixed here: "The line options should be nested under marker.line", |
Sorry, something went wrong.
|
Here are the followups for the remaining 3 comments from: #7584, @emilykl : "Could we plausibly reuse the src/traces/scatter/select.js selectPoints() function" fixed here: "Should probably call the hasColorscale() function here" fixed here: "Can you look into whether we can/should use the handleXYDefaults() function here" fixed here: |
Sorry, something went wrong.
|
Alrighty, @emilykl , thanks again for your great feedback and patience as I addressed it! I have pushed all my fixes and tested locally, I think we're close 🤞 |
Sorry, something went wrong.
|
Great news @degzhaus ! Thank you for the continued work and the clear summary. I will try to take another look within the next week. Feel free to ping me next Tuesday if I haven't gotten back to you by then. |
Sorry, something went wrong.
|
hey there, @emilykl, hope you are having a great start to the week! i just rebased and pushed again 🥳 |
Sorry, something went wrong.
|
@degzhaus would you be able to merge/rebase one more time? Sorry for the extra work. We just changed how our test images are generated and that led to the baseline conflicts with this branch. |
Sorry, something went wrong.
|
Hi @degzhaus ! I've run down the list of issues from my previous review, and most seem to be fixed 🥳 However I am still seeing these two issues:
The baseline images quiver_anchor.png, quiver_arrow-styling.png, quiver_sizemode.png, and quiver_zero-vectors.png show cut-off arrows.
The baseline images in quiver_colorscale.png and quiver_custom-colorscale.png are still all the same color. Also the quiver_colorscale should provide a value for marker.color; otherwise the arrows will not display a colorscale even if there was no bug. I'm still doing a bit more testing so I will leave another comment if I find anything else. @camdecoster is taking a look as well, so keep an eye out for his comments. Finally when you have a chance please merge/rebase against the master branch! We recently overhauled the CI pipeline and you will need to merge/rebase in order to run the new pipeline. |
Sorry, something went wrong.
| }, | ||
| anchor: { | ||
| valType: 'enumerated', | ||
| values: ['tip', 'tail', 'cm', 'center', 'middle'], |
There was a problem hiding this comment.
Could you please remove 'cm' and 'center' as values? One term for the middle should be fine.
Sorry, something went wrong.
There was a problem hiding this comment.
Much simpler, thanks, removed redundant anchor value options:
Sorry, something went wrong.
| }; | ||
|
|
||
| // Extend with base attributes (includes hoverinfo, etc.) | ||
| extendFlat(attrs, baseAttrs); |
There was a problem hiding this comment.
Did you mean to pasate baseAttrs on top of attrs? I think it would be safer to handle it this way. That's what is typically done when building attributes.
| extendFlat(attrs, baseAttrs); | |
| attrs = extendFlat({}, baseAttrs, attrs); |
Sorry, something went wrong.
There was a problem hiding this comment.
Good call, thank you, changed here:
Sorry, something went wrong.
| var trace = cd[0].trace; | ||
| var marker = trace.marker || {}; | ||
| var markerLine = marker.line || {}; | ||
| var lineColor = Lib.isArrayOrTypedArray(marker.color) ? undefined : marker.color; |
There was a problem hiding this comment.
If marker.color is an array, lineColor will be undefined. This would result in the unselected color being undefined as well. Could you update the code to handle this situation and apply a dim version (via opacity) of the selected color?
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for pointing this out. I was not able to figure out exactly how to do that with opacity, so I styled the path with stroke-opacity here:
Happy to adjust if you have something else in mind!
Sorry, something went wrong.
|
|
||
| // Colorscale cmin/cmax computation: prefer provided marker.color, else magnitude | ||
| if(trace._hasColorscale) { | ||
| var vals = hasMarkerColorArray ? [cMin, cMax] : [normMin, normMax]; |
There was a problem hiding this comment.
Could you add a guard (here or elsewhere) that handles if normMin and normMax are infinite? This is an edge case that would require all invalid data, but we should guard against it.
Sorry, something went wrong.
There was a problem hiding this comment.
Good callout, thank you, Cam, added guard here:
Sorry, something went wrong.
| var Lib = require('../../../src/lib'); | ||
| var d3Select = require('../../strict-d3').select; |
There was a problem hiding this comment.
Could you delete these unused imports?
Sorry, something went wrong.
There was a problem hiding this comment.
Absolutely, removed here:
Sorry, something went wrong.
|
@degzhaus are you still interested in finishing up this PR? |
Sorry, something went wrong.
i am, and pardon my delay, Cam! i have begun working through the recent comments, hope to be ready for another round of review this week 🤞 |
Sorry, something went wrong.
|
Great to hear! Ping us when you're ready for another review. |
Sorry, something went wrong.
|
Hello @degzhaus! We're working toward a new major release and we're interested in including this new trace as part of that. If we want that to happen, this PR will need to be merged in the next week or so. Do you think you'll have time to finish it up by then? We're happy to help if you need some assistance. |
Sorry, something went wrong.
|
@emilykl Thanks again for your diligence and help getting this across the finish line, especially the commits you've been pushing! I believe I have addressed all the open comments that you have not already addressed. Looking forward to next steps! (cc @camdecoster) |
Sorry, something went wrong.
|
Thanks @degzhaus, the updates look good! Doing a final review pass now, I might push a few additional commits if needed. |
Sorry, something went wrong.
…arrowhead drawing logic
…ta.v key name collision
…le it (determines whether arrow angle is affected by data scales of axes)
There was a problem hiding this comment.
Looks good to me!
Sorry, something went wrong.
…tly.js into degzhaus-add-quiver
|
I’m excited - are we close to merging, and is it looking like we’re going to make it into v4? |
Sorry, something went wrong.
@degzhaus Yes! There are a still a few outstanding issues I'm aware of, which I've noted in the PR description, but we're planning to release a v4.0 release candidate imminently, so we'd like to go ahead and merge this PR and address remaining issues in a follow-up PR. That means we're on track for quiver to make it into 4.0. |
Sorry, something went wrong.
|
@degzhaus Congratulations, we've merged this PR into the v4.0 branch! 🥳 🎉 We'll be putting out a release candidate for v4.0 very shortly. That doesn't mean no more changes can be made: the quiver API can be modified up until the v4.0 full release, and bug fixes can happen anytime. If you find anything that should be changed, go ahead and open up an issue, and a PR into v4.0 if you like. We'll continue working through any issues we identify as well. Thanks again for all your work on this trace type, it's been a lot of fun collaborating and we're excited to get quiver out to the public in the near future. |
Sorry, something went wrong.
Eeeeek, so exciting! Thank you so much, @emilykl for all your mentorship along the way, I learned so much on this project. And thank you @camdecoster for your help and direction as well. Really great working with you both! And thank you so much for your patience and enthusiasm @gpdf! Hope this is useful to you and your team. All the best, John |
Sorry, something went wrong.
|
@degzhaus @emilykl @camdecoster @gpdf Congrats on getting this merged - This is so exciting! There is a Plotly community forum post about this feature if you would like to chime in over there 🙂 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Overview
This PR adds a new quiver trace type to Plotly.js for visualizing 2D vector fields using arrows.
Continuation of #7584
Features
API
Plotly.newPlot('div', [{ type: 'quiver', x: [0, 1, 2], y: [0, 1, 2], u: [1, 0, -1], // x-component of vectors v: [0, 1, 0], // y-component of vectors sizemode: 'scaled', // 'scaled' | 'raw' anglemode: 'data', // 'data' | 'paper' sizeref: 0.5, anchor: 'tail', // 'tail' | 'tip' | 'center' marker: { line: { width: 2 }, colorscale: 'Viridis', }, }]);Screenshots
Examples taken from plotly.com/python/quiver-plots
Gist with example code
Quiver Plot, vector field with mesh grid (colored by magnitude)
Quiver plot, vector field with mesh grid (colored by independent color array)
Testing
Files Changed
New files (src/traces/quiver/)
Modified
Tests
Known issues
Remaining TODOs (in addition to the above)