Skip to content

Parcoords fonts #1624

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

Merged
merged 6 commits into from
May 9, 2017
Merged
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions src/traces/parcoords/attributes.js
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ var colorAttributes = require('../../components/colorscale/color_attributes');
var colorbarAttrs = require('../../components/colorbar/attributes');
var colorscales = require('../../components/colorscale/scales');
var axesAttrs = require('../../plots/cartesian/layout_attributes');
var fontAttrs = require('../../plots/font_attributes');

var extendDeep = require('../../lib/extend').extendDeep;
var extendFlat = require('../../lib/extend').extendFlat;
@@ -47,6 +48,16 @@ module.exports = {
}
},

labelfont: extendFlat({}, fontAttrs, {
description: 'Sets the font for the `dimension` labels.'
}),
tickfont: extendFlat({}, fontAttrs, {
description: 'Sets the font for the `dimension` tick values.'
}),
rangefont: extendFlat({}, fontAttrs, {
description: 'Sets the font for the `dimension` range values.'
}),

dimensions: {
_isLinkedToArray: 'dimension',
label: {
13 changes: 12 additions & 1 deletion src/traces/parcoords/defaults.js
Original file line number Diff line number Diff line change
@@ -81,7 +81,6 @@ function dimensionsDefaults(traceIn, traceOut) {
return dimensionsOut;
}


module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
function coerce(attr, dflt) {
return Lib.coerce(traceIn, traceOut, attributes, attr, dflt);
@@ -97,4 +96,16 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(!Array.isArray(dimensions) || !dimensions.length) {
traceOut.visible = false;
}

// make default font size 10px,
// scale linearly with global font size
var fontDflt = {
family: layout.font.family,
size: Math.round(layout.font.size * (10 / 12)),
Copy link
Contributor

@etpinard etpinard May 1, 2017

Choose a reason for hiding this comment

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

@monfera this is what I came off with.

As you pointed out, changing the font size default to 12 makes most parcoord graphs look less-than-optimal. We should fix that text-overlapping problem at some point though - similar to how we do it currently in cartesian axes. But that's for another PR. So, the 10px default value remains.

But, to make this a little more plotly-esque, we make the parcoord font size scale linearly with layout.font similar to what's done for axes titlefont.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @etpinard, also for the pointer!

color: layout.font.color
};

Lib.coerceFont(coerce, 'labelfont', fontDflt);
Lib.coerceFont(coerce, 'tickfont', fontDflt);
Lib.coerceFont(coerce, 'rangefont', fontDflt);
};
30 changes: 16 additions & 14 deletions src/traces/parcoords/parcoords.js
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ var lineLayerMaker = require('./lines');
var c = require('./constants');
var Lib = require('../../lib');
var d3 = require('d3');
var Drawing = require('../../components/drawing');


function keyFun(d) {return d.key;}
@@ -122,7 +123,10 @@ function model(layout, d, i) {
line = trace.line,
domain = trace.domain,
dimensions = trace.dimensions,
width = layout.width;
width = layout.width,
labelFont = trace.labelfont,
tickFont = trace.tickfont,
rangeFont = trace.rangefont;

var lines = Lib.extendDeep({}, line, {
color: lineColor.map(domainToUnitScale({values: lineColor, range: [line.cmin, line.cmax]})),
@@ -144,6 +148,9 @@ function model(layout, d, i) {
tickDistance: c.tickDistance,
unitToColor: unitToColorScale(cscale),
lines: lines,
labelFont: labelFont,
tickFont: tickFont,
rangeFont: rangeFont,
translateX: domain.x[0] * width,
translateY: layout.height - domain.y[1] * layout.height,
pad: pad,
@@ -227,8 +234,6 @@ function styleExtentTexts(selection) {
selection
.classed('axisExtentText', true)
.attr('text-anchor', 'middle')
.style('font-weight', 100)
Copy link
Contributor

@etpinard etpinard May 1, 2017

Choose a reason for hiding this comment

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

@monfera is removing that font-weight setting on purpose? Unsurprisingly, it makes the image tests fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed about font standardization... I assumed, perhaps incorrectly, that we don't want to customize it. I do have a preference for the lower font weight as in the case of parcoords the shape of line distribution is more important than the best readability so it's fine to keep the weight=100 but with configuration, the user can probably control the font anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't set font-weight anyway in our code - though inputting text as <b>text</b> effectively does that.

This is another thing I wished I would've noticed while reviewing your first parcoords PR. Oh well. I think it's best to drop font-weight and make parcoords look a little more plotly-esque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, in this case I could guess, if a bit late, how you'd go about it :-)

.style('font-size', '10px')
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha. I see 10px was used before. I should've caught that. Elsewhere in plotly.js this would be 12px.

I guess we'll have to keep it this way until v2 😬

.style('cursor', 'default')
.style('user-select', 'none');
}
@@ -556,22 +561,18 @@ module.exports = function(root, svg, styledData, layout, callbacks) {
null)
.tickFormat(d.ordinal ? function(d) {return d;} : null)
.scale(scale));
Drawing.font(axis.selectAll('text'), d.model.tickFont);
});

axis
.selectAll('.domain, .tick')
.selectAll('.domain, .tick>line')
.attr('fill', 'none')
.attr('stroke', 'black')
.attr('stroke-opacity', 0.25)
.attr('stroke-width', '1px');

axis
.selectAll('text')
.style('font-weight', 100)
.style('font-size', '10px')
.style('fill', 'black')
.style('fill-opacity', 1)
.style('stroke', 'none')
.style('text-shadow', '1px 1px 1px #fff, -1px -1px 1px #fff, 1px -1px 1px #fff, -1px 1px 1px #fff')
.style('cursor', 'default')
.style('user-select', 'none');
@@ -590,15 +591,14 @@ module.exports = function(root, svg, styledData, layout, callbacks) {
.append('text')
.classed('axisTitle', true)
.attr('text-anchor', 'middle')
.style('font-family', 'sans-serif')
.style('font-size', '10px')
.style('cursor', 'ew-resize')
.style('user-select', 'none')
.style('pointer-events', 'auto');

axisTitle
.attr('transform', 'translate(0,' + -c.axisTitleOffset + ')')
.text(function(d) {return d.label;});
.text(function(d) {return d.label;})
.each(function(d) {Drawing.font(axisTitle, d.model.labelFont);});

var axisExtent = axisOverlays.selectAll('.axisExtent')
.data(repeat, keyFun);
@@ -631,7 +631,8 @@ module.exports = function(root, svg, styledData, layout, callbacks) {
.call(styleExtentTexts);

axisExtentTopText
.text(function(d) {return formatExtreme(d)(d.domainScale.domain().slice(-1)[0]);});
.text(function(d) {return formatExtreme(d)(d.domainScale.domain().slice(-1)[0]);})
.each(function(d) {Drawing.font(axisExtentTopText, d.model.rangeFont);});

var axisExtentBottom = axisExtent.selectAll('.axisExtentBottom')
.data(repeat, keyFun);
@@ -653,7 +654,8 @@ module.exports = function(root, svg, styledData, layout, callbacks) {
.call(styleExtentTexts);

axisExtentBottomText
.text(function(d) {return formatExtreme(d)(d.domainScale.domain()[0]);});
.text(function(d) {return formatExtreme(d)(d.domainScale.domain()[0]);})
.each(function(d) {Drawing.font(axisExtentBottomText, d.model.rangeFont);});

var axisBrush = axisOverlays.selectAll('.axisBrush')
.data(repeat, keyFun);
Binary file modified test/image/baselines/gl2d_parcoords.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl2d_parcoords_1.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl2d_parcoords_2.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl2d_parcoords_blocks.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl2d_parcoords_large.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
21 changes: 19 additions & 2 deletions test/image/mocks/gl2d_parcoords_2.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
{
"layout": {
"width": 500,
"height": 500
"height": 500,
"font": {
"size": 15
}
},

"data": [{
@@ -13,13 +16,27 @@
"y": [0.2, 0.7]
},

"labelfont": {
"color": "red",
"size": 20
},
"tickfont": {
"color": "blue"
},
"rangefont": {
"color": "green"
},

"line": {
"showscale": true,
"reversescale": true,
"colorscale": "Jet",
"cmin": -4000,
"cmax": -100,
"color": [-41, -1317, -164, -1856, -79, -931, -191, -2983, -341, -3846]
"color": [-41, -1317, -164, -1856, -79, -931, -191, -2983, -341, -3846],
"colorbar": {
"x": 1.1
}
},

"dimensions": [
22 changes: 21 additions & 1 deletion test/jasmine/tests/parcoords_test.js
Original file line number Diff line number Diff line change
@@ -38,14 +38,34 @@ describe('parcoords initialization tests', function() {
expect(gd._fullData[0].opacity).toBeUndefined();
});

it('should use global font as label, tick and range font defaults', function() {
var gd = Lib.extendDeep({}, mock1);
gd.layout.font = {
family: 'Gravitas',
size: 20,
color: 'blue'
};

Plots.supplyDefaults(gd);

var expected = {
family: 'Gravitas',
size: 17,
color: 'blue'
};

expect(gd._fullData[0].labelfont).toEqual(expected);
expect(gd._fullData[0].tickfont).toEqual(expected);
expect(gd._fullData[0].rangefont).toEqual(expected);
});
});

describe('parcoords defaults', function() {

function _supply(traceIn) {
var traceOut = { visible: true },
defaultColor = '#444',
layout = { };
layout = { font: Plots.layoutAttributes.font };

Parcoords.supplyDefaults(traceIn, traceOut, defaultColor, layout);