GeezerVison

Hello, I am new to this project and attempting to contribute my first PR.

Unfortunately, I am struggling to get the build to pass. Specifically, the publish-dist-node-v22 step:
https://app.circleci.com/pipelines/github/plotly/plotly.js/12658/workflows/ae5271e7-9971-4db1-a713-b42f0625901f/jobs/282198

I tried running npm run schema and pushing the change here:
9463b3f

But still encountering the issue.

Any insight would be greatly appreciated. Thank you!

Edit: tagging a few recent commiters for visibility, thanks so much! (cc @camdecoster @alexshoe @emilykl)

Hi @degzhaus, thank you for working on this contribution!

As it stands currently, the devtools dashboard needs to be running in order for npm run schema to work properly. So you should run npm start and leave it running, and then in another terminal run npm run schema. (@camdecoster is working on a PR to eliminate that step, see #7589)

You can also download plot-schema.json from the "Artifacts" tab in CircleCI and commit it.

Hi @degzhaus, thank you for working on this contribution!

As it stands currently, the devtools dashboard needs to be running in order for npm run schema to work properly. So you should run npm start and leave it running, and then in another terminal run npm run schema. (@camdecoster is working on a PR to eliminate that step, see #7589)

You can also download plot-schema.json from the "Artifacts" tab in CircleCI and commit it.

Thank you so much, @emilykl! That was very helpful and unblocked me.

Unfortunately, I have been struggling with the no-gl-jasmine test in ci:
https://app.circleci.com/pipelines/github/plotly/plotly.js/12700/workflows/8b80de4d-1bad-455d-baef-72bc47c31042/jobs/283463

If you have any insight, I would greatly appreciate it. Thanks again!

Edit: @emilykl i figured it out! Pardon my flailing at last night. So exciting! I have a couple of tests to add and this will be ready for review.

Thanks again for your insights and help!

@degzhaus Great news, glad you're unblocked! Thank you for your work on this. Quiver is a plot type we've gotten a lot of requests for.

A few high-level comments on the API:

• This trace type is fairly distinct from existing scatter* trace types; let's go with the name quiver instead.

• The most similar already-existing trace type is the 3d cone trace, so the attributes of quiver should match the existing API of cone as closely as possible for consistency. For example, rather than the scale property it should use sizemode and sizeref as the cone trace does. (Obviously, some attributes like z and w are not relevant in a 2d context).

• Any attributes for configuring the size/scale of the arrowhead should match the arrowhead config options for annotations

  • It's also not necessary to include those options in a first version of this trace type, but any attributes that are added should be consistent with what's already in the library

• There's a few other things I noticed in the attributes as well:

  • The scaleratio attribute doesn't belong; the scale ratio between the axes should be a property of the axes themselves, not the trace. The axis scale ratio can already be configured using the existing axis.scaleratio attribute.
  • It's unclear to me how the angle attribute interacts with u and v, since u and v are already sufficient to fully determine the arrow's angle. The angle attribute should probably be removed.

@degzhaus Great news, glad you're unblocked! Thank you for your work on this. Quiver is a plot type we've gotten a lot of requests for.

A few high-level comments on the API:

• This trace type is fairly distinct from existing scatter* trace types; let's go with the name quiver instead.

• The most similar already-existing trace type is the 3d cone trace, so the attributes of quiver should match the existing API of cone as closely as possible for consistency. For example, rather than the scale property it should use sizemode and sizeref as the cone trace does. (Obviously, some attributes like z and w are not relevant in a 2d context).

• Any attributes for configuring the size/scale of the arrowhead should match the arrowhead config options for annotations

  • It's also not necessary to include those options in a first version of this trace type, but any attributes that are added should be consistent with what's already in the library

• There's a few other things I noticed in the attributes as well:

  • The scaleratio attribute doesn't belong; the scale ratio between the axes should be a property of the axes themselves, not the trace. The axis scale ratio can already be configured using the existing axis.scaleratio attribute.
  • It's unclear to me how the angle attribute interacts with u and v, since u and v are already sufficient to fully determine the arrow's angle. The angle attribute should probably be removed.

Amazing, thank you so much for taking a look and providing great guidance, Emily! Looking forward to spinning a cycle on this feedback.

@degzhaus One more comment — it would be great to include colorscale attributes in quiver so that arrows can be colored according to the magnitude of the vector. These attributes are added to the cone trace here; something similar can be done for the quiver trace. And then obviously those attributes need to be referenced in the plotting code to assign the appropriate color.

Pardon the delay, @emilykl, i was finally able to make time to clean this up. I removed some of my recent comments/questions because they were noise around me understanding how to work with the tests/images, but i was able to figure it out.

Thanks again for all your help with this PR! Excited for next steps.

Hi @degzhaus ! I'm aiming to give this another review by the end of the week. Sorry for the delay in getting back to you — looking forward to it! Thanks for your work so far.

Thank you for this idea, @gpdf ! Added here:

• 9ff8120

That's great, thank you!

Perhaps for future readers' clarity of understanding, @degzhaus , you could edit the description at the top of the PR to say:

scatterquiver quiver

in the two places it appears, so that the as-merged plot type appears there.

Perhaps for future readers' clarity of understanding, @degzhaus , you could edit the description at the top of the PR to say:

scatterquiver quiver

in the two places it appears, so that the as-merged plot type appears there.

Great catch, thank you so much, @gpdf !

Updated per your recommendation, and did a little review/revision based on changes made during the review process as well.

Hi @degzhaus ! I'm aiming to give this another review by the end of the week. Sorry for the delay in getting back to you — looking forward to it! Thanks for your work so far.

Hey there, @emilykl, hope you're having a wonderful week!

I rebased, cleaned up some code, and added some tests here:
0252e03

Thanks again for everything!

I really appreciate all the effort in polishing, and very much look forward to using this.

@emilykl Where are we on the trajectory toward getting this merged? I'm excited about being able to start using it.

@emilykl Is there anything we can do on our end to support this?

Hi @degzhaus ! I sincerely apologize for the delay in the next round of review, and a big thank you for your patience. I'll be going through this PR again this afternoon. Thanks again for your work so far and for persevering with this one!

Hi @degzhaus ! I sincerely apologize for the delay in the next round of review, and a big thank you for your patience. I'll be going through this PR again this afternoon. Thanks again for your work so far and for persevering with this one!

All good, @emilykl ! Thank you so much for the message.

I just rebased and ensured tests are passing, so looking forward to the next steps of the review.

Thanks again to you both, @emilykl @gpdf !

John

Hi @degzhaus ! Sorry for the delay. Thanks for your continued work. I remember noticing a bug on a previous iteration where the arrow styling was lost when the graph was panned; that seems to be fixed now. 🎉

However I do see some remaining items that should be addressed:

Bugs

• The axis ranges don't automatically adjust to include the full length of the arrows. See example here. By default if no axis range is specified in the layout, the entirety of all arrows should be visible. I'd recommend removing the hard-coded xaxis and yaxis range from all the image tests in the PR. Test with multiple traces as well.

• Colorscale seems broken in the latest version, unless I'm missing something. The arrows in quiver_colorscale.png and quiver_custom-colorscale.png seem to be all the same color.

• Legends are missing icons. A horizontal line icon would be fine. (A little arrow icon would be kind of cute but not required for a first iteration!)

• I can't figure out how to test the selected and unselected styling options; selecting arrows with the box styling doesn't seem to change anything. Let me know if I'm missing something. In any case these aren't essential for a first iteration, and no other trace types have them AFAIK, so it's fine to simply remove them. (Wouldn't unselected be the same as the styling set elsewhere, anyway?) Correction: I'm wrong, these do in fact exist for scatter traces! I just took a look and I understand now: unselected styling is applied to unselected points whenever any point is selected. However, I still think it's fine to skip this feature for an initial implementation of the quiver trace.

• The text property also seems broken in the latest iteration, unless I'm using it incorrectly. Could you add text to one of the image tests?

• Display of colorbars does not seem to work

API refinements (for consistency with other traces)

Open to discussion on any of these if you have an argument for keeping as-is, or another suggestion.

• c should become marker.color (similar to scatter traces and many other trace types: color can be either an array of numeric values, in which case colors are assigned according to the colorscale, or a single hex color)
  • Correspondingly, colorScaleAttrs() should be nested under marker as well
• The line options should be nested under marker.line
  • ...except for line.color, which should become simply marker.color since the markers have only a single color
  • line.shape, line.smoothing, and line.simplify should also be removed as I don't think they are relevant to quiver traces
• Move arrowheadsize under marker so that it becomes marker.arrowheadsize
  • It would also be nice if arrowheadsize imported and reused the exact attribute definition from annotations if possible
• No need for duplicate arrowwidth property since we already have line.width; arrowwidth can be removed
• arrow_scale should be removed as well

I'll leave some comments on the code as well if anything jumps out at me, but those are the big picture items! Feel free to reach out with questions on any of these, and I'll get back to you more quickly next time around. 😉 Thanks again.

Thanks so much @emilykl ! I am starting to work through your comments. One thing strange just happened when I rebased, it seemed to automatically close my PR. I have opened a new one here, but not sure what I did to cause that:

#7710

Thanks @degzhaus ! Thanks for the heads up on the closed PR, I'm not sure exactly why that happened but I've linked this PR to the new one you've opened.