Skip to content

Implement bar texts (issue 34) #4

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

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
cd2f59d
bar: add text-related attributes
n-riesco Nov 8, 2016
7d7017c
bar: draw bar texts
n-riesco Nov 8, 2016
c2cb2c8
test: bar texts
n-riesco Nov 8, 2016
2526e83
test: add mocks using bar texts
n-riesco Nov 8, 2016
a1683ee
bar: fix indentation of .enter()
n-riesco Nov 9, 2016
4124e8a
bar: increase text padding to 3px
n-riesco Nov 11, 2016
d41b74d
test: bar text nodes exist
n-riesco Nov 11, 2016
bddcaff
bar: change criteria for `textposition: 'auto'`
n-riesco Nov 11, 2016
2cbf725
test: update mock bar_text_relative
n-riesco Nov 12, 2016
1fd202c
bar: update algorithm to position bar texts
n-riesco Nov 12, 2016
6ab7a0b
bar: don't apply text padding if bar too small
n-riesco Nov 12, 2016
ffe5dae
test: remove mock bar_text_relative
n-riesco Nov 12, 2016
3455861
test: remove mock bar_text_horizontal_group
n-riesco Nov 12, 2016
a3fea79
test: remove bar_text_horizontal_relative_percent
n-riesco Nov 12, 2016
31d389c
test: remove mock bar_text_horizontal_relative
n-riesco Nov 12, 2016
af3f30b
test: update mock bar_attrs_relative
n-riesco Nov 14, 2016
a0504d1
test: update mock bar_attrs_group_norm
n-riesco Nov 14, 2016
77dc2c3
bar: fix normalised group bar plots
n-riesco Nov 14, 2016
6a78175
test: update mock bar_attrs_group_norm
n-riesco Nov 14, 2016
0289c18
bar: fix hover label
n-riesco Nov 14, 2016
3b1f162
test: update hover tests in bar_test
n-riesco Nov 14, 2016
3c2ff52
bar: update text rotation algorithm
n-riesco Nov 14, 2016
a9a13f1
test: update baseline images
n-riesco Nov 14, 2016
7d13f80
test: make linter happy
n-riesco Nov 14, 2016
aa8d68e
bar: do not coerce elements of an array attribute
n-riesco Nov 15, 2016
f8e9554
bar: accept font family, size and color arrays
n-riesco Nov 15, 2016
435b614
test: update bar font coercion tests
n-riesco Nov 15, 2016
c52f908
test: update mock bar_attrs_relative
n-riesco Nov 15, 2016
57803a0
test: update bar restyle test
n-riesco Nov 16, 2016
3e53cef
bar: bar texts only rotate clockwise
n-riesco Nov 16, 2016
ce5d53f
test: update mock bar_attrs_group_norm
n-riesco Nov 16, 2016
8cefb5b
test: update baseline images
n-riesco Nov 16, 2016
4bf9320
bar: simplify outside text positioning
n-riesco Nov 16, 2016
54272da
bar: update criteria for 'textposition: 'auto'`
n-riesco Nov 16, 2016
97699c2
test: update bar tests
n-riesco Nov 16, 2016
fea2b05
bar: describe `textposition` options
n-riesco Nov 16, 2016
cdc298d
test: update baseline images
n-riesco Nov 16, 2016
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
2 changes: 1 addition & 1 deletion src/plot_api/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ exports.cleanData = function(data, existingData) {
trace.scene = Plots.subplotsRegistry.gl3d.cleanId(trace.scene);
}

if(!Registry.traceIs(trace, 'pie')) {
if(!Registry.traceIs(trace, 'pie') && !Registry.traceIs(trace, 'bar')) {
Copy link

Choose a reason for hiding this comment

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

no need to clean (plotly.js lingo for backward-compatible mappings) attribute that have never been released.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's what this line change is meant for; i.e. to exclude bar's textposition from those mappings.

Copy link

Choose a reason for hiding this comment

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

Oops. You're right. My mistake. 😄

if(Array.isArray(trace.textposition)) {
trace.textposition = trace.textposition.map(cleanTextPosition);
}
Expand Down
28 changes: 28 additions & 0 deletions src/traces/bar/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

var scatterAttrs = require('../scatter/attributes');
var colorAttributes = require('../../components/colorscale/color_attributes');
var fontAttrs = require('../../plots/font_attributes');
var extendFlat = require('../../lib/extend').extendFlat;

var scatterMarkerAttrs = scatterAttrs.marker;
Expand All @@ -35,8 +36,35 @@ module.exports = {
y: scatterAttrs.y,
y0: scatterAttrs.y0,
dy: scatterAttrs.dy,

text: scatterAttrs.text,

textposition: {

Choose a reason for hiding this comment

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

@n-riesco what are your thoughts on adding a textinfo attribute (similar to the pie) down the road?

The default value would be 'text' which corresponds the value linked to the text attribute. Another possiblevalues could be 'total' (i.e. the total in a stack barmode).

Copy link
Owner Author

Choose a reason for hiding this comment

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

What would be the meaning of 'total' for inner bars? no label? the bar size? the cum sum up to that bar?

Choose a reason for hiding this comment

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

What would be the meaning of 'total' for inner bars?

I'm thinking 'total' would draw only one text item per bar group. But yeah, this would open up a cross-trace problem. Not so fun.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@etpinard I think this is doable. I can compute the total at the some point I determine which bars are the outmost bars.

Choose a reason for hiding this comment

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

I think this is doable

Great. That's all I wanted to here for now. No action required in this iteration.

valType: 'enumerated',
role: 'info',
values: ['inside', 'outside', 'auto', 'none'],
dflt: 'none',
arrayOk: true,
description: [
'Specifies the location of the `textinfo`.'

Choose a reason for hiding this comment

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

@n-riesco this would be a great place to add info about the inside / outside algorithm you implemented.

Choose a reason for hiding this comment

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

⬆️ blocking before making a PR to plotly.js

].join(' ')
},

textfont: extendFlat({}, fontAttrs, {
arrayOk: true,
description: 'Sets the font used for `textinfo`.'
}),

insidetextfont: extendFlat({}, fontAttrs, {
arrayOk: true,
description: 'Sets the font used for `textinfo` lying inside the bar.'
}),

outsidetextfont: extendFlat({}, fontAttrs, {
arrayOk: true,
description: 'Sets the font used for `textinfo` lying outside the bar.'
}),

orientation: {
valType: 'enumerated',
role: 'info',
Expand Down
140 changes: 139 additions & 1 deletion src/traces/bar/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

'use strict';

var isNumeric = require('fast-isnumeric');
var tinycolor = require('tinycolor2');

var Lib = require('../../lib');
var Color = require('../../components/color');

Expand All @@ -23,6 +26,125 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
return Lib.coerce(traceIn, traceOut, attributes, attr, dflt);
}

function coerceEnumerated(attributeDefinition, value, defaultValue) {
if(attributeDefinition.coerceNumber) value = +value;

if(attributeDefinition.values.indexOf(value) !== -1) return value;

return (defaultValue !== undefined) ?
defaultValue :
attributeDefinition.dflt;
}

function coerceString(attributeDefinition, value, defaultValue) {
if(typeof value === 'string') {
if(value || !attributeDefinition.noBlank) return value;
}
else if(typeof value === 'number') {
if(!attributeDefinition.strict) return String(value);
}

return (defaultValue !== undefined) ?
defaultValue :
attributeDefinition.dflt;
}

function coerceNumber(attributeDefinition, value, defaultValue) {
if(isNumeric(value)) {
value = +value;

var min = attributeDefinition.min,
max = attributeDefinition.max,
isOutOfBounds = (min !== undefined && value < min) ||
(max !== undefined && value > max);

if(!isOutOfBounds) return value;
}

return (defaultValue !== undefined) ?
defaultValue :
attributeDefinition.dflt;
}

function coerceColor(attributeDefinition, value, defaultValue) {
if(tinycolor(value).isValid()) return value;

return (defaultValue !== undefined) ?
defaultValue :
attributeDefinition.dflt;
}

function coerceFont(attributeDefinition, value, defaultValue) {
value = value || {};
defaultValue = defaultValue || {};

return {
family: coerceString(
attributeDefinition.family, value.family, defaultValue.family),
size: coerceNumber(
attributeDefinition.size, value.size, defaultValue.size),
color: coerceColor(
attributeDefinition.color, value.color, defaultValue.color)
};
}

function coerceArray(attribute, coerceFunction, defaultValue, defaultValue2) {
var attributeDefinition = attributes[attribute],
arrayOk = attributeDefinition.arrayOk,
inValue = traceIn[attribute],
inValueIsArray = Array.isArray(inValue),
defaultValueIsArray = Array.isArray(defaultValue),
outValue,
i;

// Case: inValue and defaultValue not treated as arrays
if(!arrayOk || (!inValueIsArray && !defaultValueIsArray)) {
outValue = coerceFunction(
attributeDefinition, inValue, defaultValue);
traceOut[attribute] = outValue;
return outValue;
}

// Coerce into an array
outValue = [];

// Case: defaultValue is an array and inValue isn't
if(!inValueIsArray) {
for(i = 0; i < defaultValue.length; i++) {
outValue.push(
coerceFunction(
attributeDefinition, inValue, defaultValue[i]));
}
}

// Case: inValue is an array and defaultValue isn't
else if(!defaultValueIsArray) {
for(i = 0; i < inValue.length; i++) {
outValue.push(
coerceFunction(
attributeDefinition, inValue[i], defaultValue));
}
}

// Case: inValue and defaultValue are both arrays
else {
for(i = 0; i < defaultValue.length; i++) {
outValue.push(
coerceFunction(
attributeDefinition, inValue[i], defaultValue[i]));
}
for(; i < inValue.length; i++) {
outValue.push(
coerceFunction(
attributeDefinition, inValue[i], defaultValue2));
}
}

traceOut[attribute] = outValue;

return outValue;
}

var len = handleXYDefaults(traceIn, traceOut, coerce);
if(!len) {
traceOut.visible = false;
Expand All @@ -33,7 +155,23 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('base');
coerce('offset');
coerce('width');
coerce('text');

coerceArray('text', coerceString);

var textPosition = coerceArray('textposition', coerceEnumerated);

var hasBoth = Array.isArray(textPosition) || textPosition === 'auto',
hasInside = hasBoth || textPosition === 'inside',
hasOutside = hasBoth || textPosition === 'outside';
if(hasInside || hasOutside) {
var textFont = coerceArray('textfont', coerceFont, layout.font);
if(hasInside) {
coerceArray('insidetextfont', coerceFont, textFont, layout.font);
}
if(hasOutside) {
coerceArray('outsidetextfont', coerceFont, textFont, layout.font);
}
}

Choose a reason for hiding this comment

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

It looks like you've based this on pie/defaults but you've got more logic around arrays in the input. So two questions:

  • for the cases that are covered by pie, does this give the same inheritance from layout.font and textfont to insidetextfont and outsidetextfont, particularly around cases where parts of the font come from one default and other parts come from the other one? for example if layout.font supplies the family, textfont supplies the size, and insidetextfont makes it white.
  • what extra cases does this cover and should we extend pie to cover these too?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, I've tried to replicate the behaviour of pie traces. This is tested here. The test is a good example of the extra cases handled here.

The main difference is that these attributes now also accept arrays (so that fonts can be defined per bar).

I think a lot of the functionality implemented in this file could be moved to Lib and reused by pie traces.

Choose a reason for hiding this comment

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

Cool. Looks like the inheritance for non-arrays is as I'd expect 👍 . For arrays though, I'm not sure about the structure - I'd think for consistency with data attributes and src tags, it would be better to have family, size, and color be arrays, rather than have font be an array of objects.

Copy link

Choose a reason for hiding this comment

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

We don't coerce entries inside data arrays or arrayOk attributes.

Non-text entries are usually via fallbacks later on in the pipeline e.g. in the drawing step for scatter text here.

The reason being: we try to make Plots.supplyDefaults scale ~ number of attribute as opposed to ~ length of the data.

Copy link

Choose a reason for hiding this comment

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

What the pie trace type is currently doing (here) looks sufficient to me at the default step.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@etpinard I don't understand the phrase "to make Plots.supplyDefaults scale ~ number of attribute"? Could you give me an example?

Copy link

@etpinard etpinard Nov 9, 2016

Choose a reason for hiding this comment

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

As in: there are about as many coerce statements as there are attributes.

What you added above has about as many coerce statements as there are items in the text attribute arrays.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@etpinard What are the reasons against making Plots.supplyDefaults scale with the data? Note that the calls to nestedProperty would still scale only with the number of attributes.

Having uncoerced data in arrays after running Plots.supplyDefaults means that we have to coerce this data in Bar.plot. This increases the number of places where untrusted user input can brake Plotly.

Another disadvantage of moving the code for array coercion from Bar.supplyDefaults into Bar.plot (instead of Lib) is that pie traces wouldn't be able to reuse the code.

Copy link

@etpinard etpinard Nov 9, 2016

Choose a reason for hiding this comment

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

Perf over DRY.

supplyDefaults gets called a lot more often than .plot.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@etpinard OK, I understand. Thx for the explanation.


handleStyleDefaults(traceIn, traceOut, coerce, defaultColor, layout);

Expand Down
Loading