Conversation
|
To record my verbal comments from earlier: this looks right from the point of view of The tricky bit is that this would include all the colorscales in |
|
I need to add a couple of tests as well, but let's see first what percy thinks about this (I did not include the reversed colorscales in the swatches, so there should be no change). |
| hovertemplate="%{y}[%{customdata}] = %{marker.color}<extra></extra>", | ||
| ) | ||
| for name, colors in reversed(sequences) | ||
| for name, colors in (sequences) |
There was a problem hiding this comment.
i actually kind of liked the old order, otherwise they read bottom up...
There was a problem hiding this comment.
oh good catch, I had modified this while debugging and then forgot to change it back.
|
looks great to me! could you revert the codegen'ed piece of this plz though leaving just the changes to the core lib in color an util? 💃 |
|
Not sure I understand your comments about the codegened piece: a lot of files are modified because docstrings are modified. Shouldn't we keep these modifications? Or are they going to be rolled out by something else? (the release etc.). Or should I just revert the |
jonmmease
left a comment
jonmmease
left a comment
There was a problem hiding this comment.
Thanks @emmanuelle. I added a few comments of things that I don't think should be changed in this PR. Let me know if it doesn't make sense.
| `outliercolor`) If "all", all sample points are shown | ||
| If False, only the violins are shown with no sample | ||
| points and the whiskers extend to the range of the sample. | ||
| points |
There was a problem hiding this comment.
@emmanuelle, I don't think this docstring change should be coming in this PR. Maybe try rerunning codegen in case you had a different schema around when you generated this.
There was a problem hiding this comment.
Actually #1988 already removed those. I think they were not added at the right place (I think it was a community PR, I must have merged it without thinking that this should have been in the codegen instead.)
There was a problem hiding this comment.
What do you suggest to do here @jonmmease ?
There was a problem hiding this comment.
It looks like this code was somehow generated from the schema for plotly.js 1.51.2 (which this branch shouldn't have yet). In any case, why don't you try merging in master and then running codegen again. Hopefully then the diff will only show the colorscale docstring changes.
| @@ -9,26 +9,26 @@ | |||
| "resolved": "https://registry.npmjs.org/3d-view/-/3d-view-2.0.0.tgz", | |||
There was a problem hiding this comment.
It shouldn't be necessary for this file to have changed unless you're changing the bundled version of plotly.js. Could you revert it?
There was a problem hiding this comment.
This one will go away when rebasing on master. I don't think I changed the bundled version of plotly.js but I ran the codegen to update the docstrings.
There was a problem hiding this comment.
When you run codegen, are you using python setup.py codegen? This shouldn't trigger any JavaScript/npm logic that would alter this file. When you merge in master, go ahead and accept masters version of this file as-is.
| @@ -10495,27 +10495,27 @@ module.exports = function(opts) { | |||
| /* 10 */ | |||
There was a problem hiding this comment.
Revert these changes as well
|
OK, I ended up with a big git mess. I'll work on it later this evening, hope it won't be too late |
|
No worries, I'll do the release tomorrow morning. Thanks for working through it! |
|
Not there yet but making progress :-) |
|
Hey @jonmmease I think this one is ready now. Thank you for your patience! |
|
Hum, not quite yet, CI not happy. |
|
Thanks @emmanuelle! I made one small testing change in 0c555f6 to allow all of the |
3 participants
Closes #1681