GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add quiver trace type for vector field visualization by degzhaus · Pull Request #7710 · plotly/plotly.js Conversation
|
|
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: " "The "Display of colorbars does not seem to work" fixed here: -- API Refinements -- " "The line options should be nested under marker.line", |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
acfd7f0 to
1a15aa3
Compare
|
hey there, @emilykl, hope you are having a great start to the week! i just rebased and pushed again 🥳 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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
The baseline images in 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.
There was an error while loading. Please reload this page.
| }, | ||
| anchor: { | ||
| valType: 'enumerated', | ||
| values: ['tip', 'tail', 'cm', 'center', 'middle'], |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove 'cm' and 'center' as values? One term for the middle should be fine.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Much simpler, thanks, removed redundant anchor value options:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| }; | ||
|
|
||
| // Extend with base attributes (includes hoverinfo, etc.) | ||
| extendFlat(attrs, baseAttrs); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Good call, thank you, changed here:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
There was an error while loading. Please reload this page.
|
|
||
| // 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.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Good callout, thank you, Cam, added guard here:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| var Lib = require('../../../src/lib'); | ||
| var d3Select = require('../../strict-d3').select; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Could you delete these unused imports?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, removed here:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@degzhaus are you still interested in finishing up this PR? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
There was an error while loading. Please reload this page.
|
Great to hear! Ping us when you're ready for another review. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
|
@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.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
…arrowhead drawing logic
…ta.v key name collision
…le it (determines whether arrow angle is affected by data scales of axes)
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
0d490dc to
731112d
Compare
…tly.js into degzhaus-add-quiver
731112d to
d8d8697
Compare
|
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.
There was an error while loading. Please reload this page.
@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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@degzhaus Congratulations, we've merged this PR into the 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 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.
There was an error while loading. Please reload this page.
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.
There was an error while loading. Please reload this page.
|
@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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.