Remove pattern from funnel attributes and add tests for histogram and barpolar #5537
Conversation
src/traces/funnel/style.js
Outdated
| } | ||
| }); | ||
|
|
||
| Drawing.pointStyle(gTrace.selectAll('path'), trace, gd); |
There was a problem hiding this comment.
Does this call make the loop above, or some of it, moot?
There was a problem hiding this comment.
The trace variable uses each loop.
I think this part is similar to what bar does here.
There was a problem hiding this comment.
It just looked to me as though at least the Color.fill and Color.stroke calls (L27-28) might be duplicated by pointStyle. Not Drawing.dashLine and I can't really tell about opacity... but the point is Drawing.pointStyle does more than apply a pattern, yet the other things it does already worked for funnels prior to this PR.
There was a problem hiding this comment.
Actually deselect style is broken on funnel now.
Also strangely if you make a selection on barpolar traces of pattern_bars mock, the points from bar traces would be selected!
There was a problem hiding this comment.
@s417-lama I am about to log off for today. Would you mind investigating this part?
There was a problem hiding this comment.
I investigated the selection issue a little bit.
Seems like determining candidates for selection (determineSearchTraces function in src/plots/cartesian/select.js) is broken when polar plot and other plots coexist.
When we try to select barpolar plots, bar plots are wrongly included in the candidate lists for selection.
In determineSearchTraces fn, the following line is executed for bar plots, which is an inappropriate behavior.
plotly.js/src/plots/cartesian/select.js
Line 663 in 3f33829
This bug happens because the values of xAxisIds and yAxisIds are set to ["x"] and ["y"] in barpolar plots, but by nature polar plots should not have cartesian axes. As a result, the plot which has x and y axes, that is bar plot in this case, is incorrectly included in searchTraces. I suggest to clear x-axes and y-axes in case of barpolar plots somewhere in codes.
There was a problem hiding this comment.
pattern removed from funnel attributes in 4c4828a. So there is no need to investigate this in the PR.
|
Let's land this this week if possible and have pattern fill in 2.0 :) |
alexcjohnson
left a comment
There was a problem hiding this comment.
Just two minor comments - looks great! 💃
Co-authored-by: Alex Johnson <[email protected]>
Follow up of #5520
this PR adds tests for
histogramandbarpolarand refactorspatternlogic frombartodrawingandlib.@plotly/plotly_js
cc: @s417-lama