The Wayback Machine - https://web.archive.org/web/20210309024112/https://github.com/plotly/plotly.js/pull/5520
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pattern fill for histogram, bar and barpolar traces #5520

Merged
merged 6 commits into from Mar 8, 2021

Conversation

@s417-lama
Copy link
Contributor

@s417-lama s417-lama commented Feb 23, 2021

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 color
  • scale: scaling factor of the preset shape of patterns
  • solidity: line width or radius of dots

Sample figure:

bar_patternfill

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Feb 23, 2021

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

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Feb 24, 2021

@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!

s417-lama and others added 2 commits Mar 3, 2021
Co-authored-by: Alex Johnson <[email protected]>
Copy link
Contributor

@alexcjohnson alexcjohnson left a 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) {

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

Could you declare this function outside the switch case please?

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

Fixed in 5161478

@@ -482,6 +670,16 @@ drawing.singlePointStyle = function(d, sel, trace, fns, gd) {
if(!gradientInfo[gradientType]) gradientType = 0;
}

var getPatternAttr = function(mp, i, dflt) {

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

Non-blocking:
It looks like you could also move this function above drawing.pointStyle.

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

Fixed in 5161478

var getPatternAttr = function(mp, i, dflt) {
if(mp && Array.isArray(mp)) {
if(i < mp.length) return mp[i];
else return dflt;

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

How about simply

return i < mp.length ? mp[i] : dflt;

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

Fixed in 5161478


var fillColor = d0.mc || marker.color;

var getPatternAttr = function(mp, dflt) {

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

This function looks quite similar to getPatternAttr in src/components/drawing/index.js.
Could you explain the difference between two?

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

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?

This comment has been minimized.

@archmoj

archmoj Mar 8, 2021
Collaborator

It could go in drawing.

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

Sure! Fixed in 5161478

}
});
}
for(k in fullLayout._gradientUrlQueryParts) queryParts.push(k);

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

Can't we use slice instead of the for loop here and below?

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

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?

@@ -23,6 +23,12 @@ module.exports = function handleStyleDefaults(traceIn, traceOut, coerce, default

coerce('marker.line.width');
coerce('marker.opacity');
var pattern = coerce('marker.pattern.shape');

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

Let's call this variable patternShape.

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

Fixed in 5161478

if(pattern) {
coerce('marker.pattern.bgcolor');
coerce('marker.pattern.size');
coerce('marker.pattern.solidity');

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

We need to add a supplyDefaults test somewhere here for these attributes not be coerced when there is no pattern.shape.

@@ -39,6 +39,54 @@ var marker = extendFlat({
max: 1,
editType: 'style',
description: 'Sets the opacity of the bars.'
},
pattern: {

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

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?

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

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);

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

Shouldn't we use Lib.isArrayOrTypedArray here?

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

Fixed in 5161478

var fillColor = d0.mc || marker.color;

var getPatternAttr = function(mp, dflt) {
if(mp && Array.isArray(mp)) {

This comment has been minimized.

@archmoj

archmoj Mar 3, 2021
Collaborator

Should we handle typedArrays here as well?

This comment has been minimized.

@s417-lama

s417-lama Mar 8, 2021
Author Contributor

Fixed in 5161478

@archmoj archmoj added this to the NEXT milestone Mar 4, 2021
@archmoj
Copy link
Collaborator

@archmoj archmoj commented Mar 4, 2021

This PR also added pattern feature to histogram trace, which is cool!

Plotly.PlotSchema.get().traces.histogram.attributes.marker.pattern

So, let's add/expand bar image & jasmine tests to histogram and edit PR title & description.

@archmoj
Copy link
Collaborator

@archmoj archmoj commented Mar 4, 2021

This PR also added pattern attribute to the schema of funnel trace marker.
To get that to work you need to coerce the new attributes in src/traces/funnel/defaults.js,
then add

Drawing.pointStyle(gTrace.selectAll('path'), trace, gd);

above this line

styleTextPoints(gTrace, trace, gd);
@archmoj
Copy link
Collaborator

@archmoj archmoj commented Mar 4, 2021

This PR also added pattern feature to barpolar trace, which is cool!

Plotly.PlotSchema.get().traces.barpolar.attributes.marker.pattern

So, let's add/expand bar image & jasmine tests to barpolar as well and edit PR title & description.

@s417-lama
Copy link
Contributor Author

@s417-lama s417-lama commented Mar 8, 2021

I will handle the remaining issues (some tests and supports for plots other than bar plots) when I have time.

@archmoj
Copy link
Collaborator

@archmoj archmoj commented Mar 8, 2021

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.
@s417-lama wondering if you do not you have time to address those remanings today, I could merge this PR and help fix those?

@s417-lama
Copy link
Contributor Author

@s417-lama s417-lama commented Mar 8, 2021

@archmoj I don't think I have enough time today. Could you help fixing the remaining things? Thank you!

@archmoj
Copy link
Collaborator

@archmoj archmoj commented Mar 8, 2021

@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.
Again thanks very much for the remarkable contribution! 🥇
💃

@archmoj archmoj changed the title Add pattern fill for bar plots Add pattern fill for histogram, bar and barpolar traces Mar 8, 2021
@archmoj archmoj merged commit c80fda4 into plotly:master Mar 8, 2021
9 checks passed
9 checks passed
ci/circleci: flaky-image Your tests passed on CircleCI!
Details
ci/circleci: install-and-cibuild Your tests passed on CircleCI!
Details
ci/circleci: jasmine-bundle Your tests passed on CircleCI!
Details
ci/circleci: no-gl-flaky-jasmine Your tests passed on CircleCI!
Details
ci/circleci: no-gl-jasmine Your tests passed on CircleCI!
Details
ci/circleci: publish-dist Your tests passed on CircleCI!
Details
ci/circleci: source-syntax Your tests passed on CircleCI!
Details
ci/circleci: stable-image Your tests passed on CircleCI!
Details
ci/circleci: webgl-jasmine Your tests passed on CircleCI!
Details
@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Mar 8, 2021

Amazing contribution, thank you so much! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants