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

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

@archmoj archmoj added status: reviewable community community contribution feature something new labels Feb 23, 2021
@nicolaskruchten
Copy link
Contributor

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
Collaborator

@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 March 3, 2021 15:14
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you declare this function outside the switch case please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5161478

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about simply

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5161478


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

var getPatternAttr = function(mp, dflt) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could go in drawing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Fixed in 5161478

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this variable patternShape.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5161478

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use Lib.isArrayOrTypedArray here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5161478

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle typedArrays here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5161478

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

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
Contributor

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
Contributor

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

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

@archmoj
Copy link
Contributor

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

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

@archmoj
Copy link
Contributor

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
@nicolaskruchten
Copy link
Contributor

Amazing contribution, thank you so much! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants