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.

Sign up for GitHub

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

Remove pattern from funnel attributes and add tests for histogram and barpolar #5537

Merged
merged 9 commits into from
Apr 21, 2021
Merged

Remove pattern from funnel attributes and add tests for histogram and barpolar #5537

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b0d186d
get added patten to work for funnel
archmoj Mar 8, 2021
9335168
move pattern to drawing and make a lib fn to coerce it
archmoj Mar 8, 2021
1bddefa
rename bar_patternfill to pattern_bars
archmoj Mar 8, 2021
8334830
improve pattern image test
archmoj Mar 8, 2021
fc1adf5
improve pattern mock
archmoj Mar 9, 2021
4c4828a
drop pattern from funnel marker
archmoj Apr 16, 2021
0c27bbb
use extendFlat
archmoj Apr 16, 2021
496c643
Update src/components/drawing/attributes.js
archmoj Apr 21, 2021
4de929d
do not make and return pattern object from coercePattern - unused for…
archmoj Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/components/drawing/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,52 @@ exports.dash = {
'or a dash length list in px (eg *5px,10px,2px,2px*).'
].join(' ')
};

exports.pattern = {
shape: {
valType: 'enumerated',
values: ['', '/', '\\', 'x', '-', '|', '+', '.'],
dflt: '',
arrayOk: true,
editType: 'style',
description: [
'Sets the shape of the pattern fill.',
'By default, no pattern is used for filling the area.',
].join(' ')
},
bgcolor: {
valType: 'color',
arrayOk: true,
editType: 'style',
description: [
'Sets the background color of the pattern fill.',
'Defaults to a transparent background.',
].join(' ')
},
size: {
valType: 'number',
min: 0,
dflt: 8,
arrayOk: true,
editType: 'style',
description: [
'Sets the size of unit squares of the pattern fill in pixels,',
'which corresponds to the interval of repetition of the pattern.',
].join(' ')
},
solidity: {
valType: 'number',
min: 0,
max: 1,
dflt: 0.3,
arrayOk: true,
editType: 'style',
description: [
'Sets the solidity of the pattern fill.',
'Solidity is roughly proportional to the ratio of the area filled by the pattern.',
'Solidity of 0 shows only the background color without pattern',
'and solidty of 1 shows only the foreground color without pattern.',
].join(' ')
},
editType: 'style'
};
16 changes: 16 additions & 0 deletions src/lib/coerce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,22 @@ exports.coerceFont = function(coerce, attr, dfltObj) {
return out;
};

/*
* Shortcut to coerce the pattern attributes
*/
exports.coercePattern = function(coerce, attr) {
var out = {};

out.shape = coerce(attr + '.shape');
if(out.shape) {
out.size = coerce(attr + '.size');
out.bgcolor = coerce(attr + '.bgcolor');
out.solidity = coerce(attr + '.solidity');
}

return out;
};

/** Coerce shortcut for 'hoverinfo'
* handling 1-vs-multi-trace dflt logic
*
Expand Down
1 change: 1 addition & 0 deletions src/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ lib.valObjectMeta = coerceModule.valObjectMeta;
lib.coerce = coerceModule.coerce;
lib.coerce2 = coerceModule.coerce2;
lib.coerceFont = coerceModule.coerceFont;
lib.coercePattern = coerceModule.coercePattern;
lib.coerceHoverinfo = coerceModule.coerceHoverinfo;
lib.coerceSelectionMarkerOpacity = coerceModule.coerceSelectionMarkerOpacity;
lib.validate = coerceModule.validate;
Expand Down
50 changes: 2 additions & 48 deletions src/traces/bar/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var texttemplateAttrs = require('../../plots/template_attributes').texttemplateA
var colorScaleAttrs = require('../../components/colorscale/attributes');
var fontAttrs = require('../../plots/font_attributes');
var constants = require('./constants');
var pattern = require('../../components/drawing/attributes').pattern;

var extendFlat = require('../../lib/extend').extendFlat;

Expand Down Expand Up @@ -40,54 +41,7 @@ var marker = extendFlat({
editType: 'style',
description: 'Sets the opacity of the bars.'
},
pattern: {
shape: {
valType: 'enumerated',
values: ['', '/', '\\', 'x', '-', '|', '+', '.'],
dflt: '',
arrayOk: true,
editType: 'style',
description: [
'Sets the shape of the pattern fill.',
'By default, no pattern is used for filling the area.',
].join(' ')
},
bgcolor: {
valType: 'color',
arrayOk: true,
editType: 'style',
description: [
'Sets the background color of the pattern fill.',
'Defaults to a transparent background.',
].join(' ')
},
size: {
valType: 'number',
min: 0,
dflt: 8,
arrayOk: true,
editType: 'style',
description: [
'Sets the size of unit squares of the pattern fill in pixels,',
'which corresponds to the interval of repetition of the pattern.',
].join(' ')
},
solidity: {
valType: 'number',
min: 0,
max: 1,
dflt: 0.3,
arrayOk: true,
editType: 'style',
description: [
'Sets the solidity of the pattern fill.',
'Solidity is roughly proportional to the ratio of the area filled by the pattern.',
'Solidity of 0 shows only the background color without pattern',
'and solidty of 1 shows only the foreground color without pattern.',
].join(' ')
},
editType: 'style'
}
pattern: pattern
});

module.exports = {
Expand Down
9 changes: 3 additions & 6 deletions src/traces/bar/style_defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
var Color = require('../../components/color');
var hasColorscale = require('../../components/colorscale/helpers').hasColorscale;
var colorscaleDefaults = require('../../components/colorscale/defaults');
var coercePattern = require('../../lib').coercePattern;

module.exports = function handleStyleDefaults(traceIn, traceOut, coerce, defaultColor, layout) {
coerce('marker.color', defaultColor);
Expand All @@ -23,12 +24,8 @@ module.exports = function handleStyleDefaults(traceIn, traceOut, coerce, default

coerce('marker.line.width');
coerce('marker.opacity');
var patternShape = coerce('marker.pattern.shape');
if(patternShape) {
coerce('marker.pattern.bgcolor');
coerce('marker.pattern.size');
coerce('marker.pattern.solidity');
}
coercePattern(coerce, 'marker.pattern');

coerce('selected.marker.color');
coerce('unselected.marker.color');
};
1 change: 1 addition & 0 deletions src/traces/funnel/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
var markerColor = coerce('marker.color', defaultColor);
coerce('marker.line.color', Color.defaultLine);
coerce('marker.line.width');
Lib.coercePattern(coerce, 'marker.pattern');

var connectorVisible = coerce('connector.visible');
if(connectorVisible) {
Expand Down
1 change: 1 addition & 0 deletions src/traces/funnel/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ function style(gd, cd, sel) {
}
});

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

Choose a reason for hiding this comment

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

Does this call make the loop above, or some of it, moot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trace variable uses each loop.
I think this part is similar to what bar does here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s417-lama I am about to log off for today. Would you mind investigating this part?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

searchTraces.push(createSearchInfo(trace._module, cd,

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pattern removed from funnel attributes in 4c4828a. So there is no need to investigate this in the PR.

styleTextPoints(gTrace, trace, gd);

gTrace.selectAll('.regions').each(function() {
Expand Down
Binary file removed test/image/baselines/bar_patternfill.png
View file
Open in desktop
Binary file not shown.
Binary file added test/image/baselines/pattern_bars.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
125 changes: 0 additions & 125 deletions test/image/mocks/bar_patternfill.json
View file
Open in desktop

This file was deleted.

Loading