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.

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

Merged
merged 6 commits into from
May 9, 2017
Merged

Parcoords fonts #1624

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
505614d
parcoords fonts
monfera Apr 26, 2017
9939241
specify default sizes for parcoords fonts; screenshot fix
monfera Apr 26, 2017
a8e444a
avoid duplicate coercion
monfera Apr 26, 2017
3925d6d
make parcoord font size dflt scale linearly with global font
etpinard May 1, 2017
a3544c7
showcase new parcoord font attributes in `parcoords_2` mock
etpinard May 1, 2017
c0e0845
update parcoords baselines (:hocho: font-weight)
etpinard May 1, 2017
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
6 changes: 6 additions & 0 deletions src/traces/parcoords/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ 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;


Copy link
Contributor

Choose a reason for hiding this comment

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

🔪

Copy link
Contributor

Choose a reason for hiding this comment

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

... as we're (slowly) trying to make our code look more like the standard style - which disallows multiple blank lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got it, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw HUGE fan of standard style

Copy link
Contributor

@rreusser rreusser Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Now that there aren't giant outstanding PRs, might be time to revisit: #1371 (though I'm increasingly intrigued by prettier which is fundamentally different from a linter)

Copy link
Contributor

@rreusser rreusser Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

--> The selling point of prettier is that you don't have to adhere to any style at all because it doesn't present you with errors. It just makes things consistent. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

True. But you have to do some non-trivial src vs build file trickery.

Copy link
Contributor

Choose a reason for hiding this comment

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

... adding pre-commit hooks is hardly simplifying anything IMHO.

Copy link
Contributor

@rreusser rreusser Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

See #1629 for quick experiment. It replaces lint and lint-fix with prettier equivalents and adds a precommit hook that formats staged files without ever having to manually add a precommit hook.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're bikeshedding #950 (comment)

module.exports = {

domain: {
Expand Down Expand Up @@ -47,6 +49,10 @@ module.exports = {
}
},

labelfont: extendFlat({}, fontAttrs, {dflt: {size: 10}, description: 'Sets the font for the `dimension` labels.'}),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not our usual pattern.

The default font size is inherited from fullLayout.font.size directly as here or times a scalar like 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.

The purpose was to have smaller fonts than what would come from the layout defaults, which would result in overly big numbers, looked really bad. The dimension labels and tick numbers need to be quite small by default, e.g. around 10px. It's achieved by parcoords/attributes specifying these sizes. Is there another way of saying, 'take layout font values but don't adopt large fonts unless the user wants it, in which case they specify it on eg. labelfont?

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

dimensions: {
_isLinkedToArray: 'dimension',
label: {
Expand Down
7 changes: 7 additions & 0 deletions src/traces/parcoords/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ function dimensionsDefaults(traceIn, traceOut) {
return dimensionsOut;
}

function coerceFont(fontAttr, coerce, layoutFont, defaultFont) {
Lib.coerceFont(coerce, fontAttr, Lib.extendFlat({}, layoutFont, defaultFont, fontAttr));
}

module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
function coerce(attr, dflt) {
Expand All @@ -97,4 +100,8 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(!Array.isArray(dimensions) || !dimensions.length) {
traceOut.visible = false;
}

coerceFont('labelfont', coerce, layout.font, attributes.labelfont.dflt || {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Lib.coerceFont('labelfont', coerce, layout.font);

should be enough, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do that. The consequence is that the tick and dimension label fonts would be too large:
image

It's even worse if the user specified a larger font centrally, e.g. size 16:
image

Since I can go either way, pls confirm if I should preserve the current fontsize-moderation logic or use the simpler logic. Consistency matters a lot on one hand, and having ergonomically good defaults is also valuable.

coerceFont('tickfont', coerce, layout.font, attributes.tickfont.dflt || {});
coerceFont('rangefont', coerce, layout.font, attributes.rangefont.dflt || {});
};
30 changes: 16 additions & 14 deletions src/traces/parcoords/parcoords.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;}
Expand Down Expand Up @@ -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]})),
Expand All @@ -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,
Expand Down Expand Up @@ -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 •
edited
Loading

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');
}
Expand Down Expand Up @@ -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');
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down