Add pattern fill for histogram, bar and barpolar traces - #5520
Conversation
|
Wow, awesome 😍 ! For reference, here is what Bokeh supports, including their little one-character abbreviations, for things we could add later: https://docs.bokeh.org/en/latest/docs/user_guide/styling.html#hatch-properties |
|
@s417-lama very nice work! I've made a few comments but I think you're off to a great start, and this will be a popular feature! |
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
alexcjohnson
left a comment
There was a problem hiding this comment.
Thanks for the updates @s417-lama - this looks ready to go from my side! 💃 @archmoj anything else from you?
The no-gl-jasmine test failure was one I had never seen before, it just couldn't find the test modules somehow? Anyhow it succeeded the second time.
| radius = Math.sqrt(solidity * size * size / Math.PI); | ||
| } else { | ||
| // linear interpolation | ||
| var linearFn = function(x, x0, x1, y0, y1) { |
There was a problem hiding this comment.
Could you declare this function outside the switch case please?
| if(!gradientInfo[gradientType]) gradientType = 0; | ||
| } | ||
|
|
||
| var getPatternAttr = function(mp, i, dflt) { |
There was a problem hiding this comment.
Non-blocking:
It looks like you could also move this function above drawing.pointStyle.
| var getPatternAttr = function(mp, i, dflt) { | ||
| if(mp && Array.isArray(mp)) { | ||
| if(i < mp.length) return mp[i]; | ||
| else return dflt; |
There was a problem hiding this comment.
How about simply
return i < mp.length ? mp[i] : dflt;|
|
||
| var fillColor = d0.mc || marker.color; | ||
|
|
||
| var getPatternAttr = function(mp, dflt) { |
There was a problem hiding this comment.
This function looks quite similar to getPatternAttr in src/components/drawing/index.js.
Could you explain the difference between two?
There was a problem hiding this comment.
getPatternAttr in legends does not require any index because the first element of the array is always used, but the same getPatternAttr function can be used for both. If we unify them, which location do you think is appropriate for putting the common getPatternAttr fn?
| } | ||
| }); | ||
| } | ||
| for(k in fullLayout._gradientUrlQueryParts) queryParts.push(k); |
There was a problem hiding this comment.
Can't we use slice instead of the for loop here and below?
There was a problem hiding this comment.
In this regard, I couldn't see the point in storing query parts as a dict (_gradientUrlQueryParts) in the first place. Seems like only keys are used and values are not. Why don't we make it just an array?
Also, I suspect that it makes things more complicated to store query parts globally. How about just putting a dedicated class name for pattern fill (and gradient) and collecting them later? Actually, I already did the similar thing (below).
in components/drawing/index.js:
sel.classed('pattern_filled', true);
var className2query = function(s) {
return '.' + s.attr('class').replace(/\s/g, '.');
};
var k = className2query(d3.select(sel.node().parentNode)) + '>.pattern_filled';
fullLayout._patternUrlQueryParts[k] = 1;I think it is enough to put .pattern_filled class and collect them by using a query for .pattern_filled when exporting svg. Maybe I am missing some points; is there any problem with it?
|
|
||
| coerce('marker.line.width'); | ||
| coerce('marker.opacity'); | ||
| var pattern = coerce('marker.pattern.shape'); |
There was a problem hiding this comment.
Let's call this variable patternShape.
| if(pattern) { | ||
| coerce('marker.pattern.bgcolor'); | ||
| coerce('marker.pattern.size'); | ||
| coerce('marker.pattern.solidity'); |
There was a problem hiding this comment.
We need to add a supplyDefaults test somewhere here for these attributes not be coerced when there is no pattern.shape.
| editType: 'style', | ||
| description: 'Sets the opacity of the bars.' | ||
| }, | ||
| pattern: { |
There was a problem hiding this comment.
The bar/attributes is required and used by other traces namely barpolar, box, histogram, funnel, waterfall.
Are we planning to add pattern to any of those?
There was a problem hiding this comment.
Technically it should be quite easy to apply pattern fill to other plots. I'm planning to support all of them and add some tests!
| var perPointPattern = Array.isArray(markerPattern.shape) || | ||
| Array.isArray(markerPattern.bgcolor) || | ||
| Array.isArray(markerPattern.size) || | ||
| Array.isArray(markerPattern.solidity); |
There was a problem hiding this comment.
Shouldn't we use Lib.isArrayOrTypedArray here?
| var fillColor = d0.mc || marker.color; | ||
|
|
||
| var getPatternAttr = function(mp, dflt) { | ||
| if(mp && Array.isArray(mp)) { |
There was a problem hiding this comment.
Should we handle typedArrays here as well?
|
This PR also added Plotly.PlotSchema.get().traces.histogram.attributes.marker.patternSo, let's add/expand |
|
This PR also added Drawing.pointStyle(gTrace.selectAll('path'), trace, gd);above this line plotly.js/src/traces/funnel/style.js Line 34 in ed52205 |
|
This PR also added Plotly.PlotSchema.get().traces.barpolar.attributes.marker.patternSo, let's add/expand |
|
I will handle the remaining issues (some tests and supports for plots other than bar plots) when I have time. |
We would love to include this feature in the upcoming v2.0.0-rc.1. |
|
@archmoj I don't think I have enough time today. Could you help fixing the remaining things? Thank you! |
Sure. I'll take care of it. |
|
Amazing contribution, thank you so much! 🙇 |
Added pattern fill for bar plots ( #3815 ).
Although pattern fill could be applied to various types of plots, for now only bar plots are supported as a first step.
Supported attributes:
shape: borders (/,\,|,-), cross hatch (+,x), dots (.)bgcolor: background colorscale: scaling factor of the preset shape of patternssolidity: line width or radius of dotsSample figure: