Parcoords fonts #1624

monfera · 2017-04-26T16:39:00Z

Support for specifying fonts for parcoords dimensions. Example, see JS code 8..12:http://codepen.io/anon/pen/QvdwpX

etpinard · 2017-04-26T18:00:42Z

src/traces/parcoords/defaults.js

@@ -81,6 +81,10 @@ function dimensionsDefaults(traceIn, traceOut) {
    return dimensionsOut;
 }

+function coerceFont(fontAttr, coerce, layoutFont, defaultFont) {
+    var fontSpec = Lib.coerceFont(coerce, fontAttr);
+    Lib.coerceFont(coerce, fontAttr, Lib.extendFlat({}, layoutFont, defaultFont, fontSpec));


Why do you need to call Lib.coerceFont twice?

etpinard · 2017-04-26T21:11:06Z

src/traces/parcoords/attributes.js


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

+


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

Ah got it, thanks!

Btw HUGE fan of standard style

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)

--> 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. 😄

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

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

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.

I think we're bikeshedding #950 (comment)

etpinard · 2017-04-26T21:14:29Z

src/traces/parcoords/attributes.js

@@ -47,6 +49,10 @@ module.exports = {
        }
    },

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


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.

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?

etpinard · 2017-04-26T21:15:56Z

src/traces/parcoords/defaults.js

@@ -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 || {});


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

should be enough, no?

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

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

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.

etpinard · 2017-04-26T21:17:58Z

src/traces/parcoords/parcoords.js

@@ -227,8 +234,6 @@ function styleExtentTexts(selection) {
    selection
        .classed('axisExtentText', true)
        .attr('text-anchor', 'middle')
-        .style('font-weight', 100)
-        .style('font-size', '10px')


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 😬

etpinard · 2017-05-01T15:56:01Z

src/traces/parcoords/parcoords.js

@@ -227,8 +234,6 @@ function styleExtentTexts(selection) {
    selection
        .classed('axisExtentText', true)
        .attr('text-anchor', 'middle')
-        .style('font-weight', 100)


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

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.

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.

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

- N.B. this keeps the 10px default

etpinard · 2017-05-01T17:31:39Z

src/traces/parcoords/defaults.js

+    // scale linearly with global font size
+    var fontDflt = {
+        family: layout.font.family,
+        size: Math.round(layout.font.size * (10 / 12)),


@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.

Thanks @etpinard, also for the pointer!

etpinard · 2017-05-09T14:19:23Z

@monfera are you ok with my changes?

monfera · 2017-05-09T14:22:39Z

@etpinard yes, thank you, I should have indicated this more unambiguously in my reply.

monfera · 2017-05-09T14:23:22Z

Also, overall, looks good!

monfera added 2 commits April 26, 2017 11:45

parcoords fonts

505614d

specify default sizes for parcoords fonts; screenshot fix

9939241

monfera self-assigned this Apr 26, 2017

etpinard reviewed Apr 26, 2017

View reviewed changes

etpinard added status: has TODOs type: feature labels Apr 26, 2017

avoid duplicate coercion

a8e444a

etpinard reviewed Apr 26, 2017

View reviewed changes

etpinard added this to the 1.27.0 milestone Apr 26, 2017

monfera assigned etpinard May 1, 2017

etpinard reviewed May 1, 2017

View reviewed changes

etpinard added 3 commits May 1, 2017 13:24

make parcoord font size dflt scale linearly with global font

3925d6d

- N.B. this keeps the 10px default

showcase new parcoord font attributes in parcoords_2 mock

a3544c7

update parcoords baselines (:hocho: font-weight)

c0e0845

etpinard reviewed May 1, 2017

View reviewed changes

etpinard added status: reviewable and removed status: has TODOs labels May 1, 2017

etpinard merged commit 1ffb102 into master May 9, 2017

etpinard deleted the parcoords-fonts branch May 9, 2017 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parcoords fonts #1624

Parcoords fonts #1624

monfera commented Apr 26, 2017

etpinard Apr 26, 2017

etpinard Apr 26, 2017

etpinard Apr 26, 2017

monfera Apr 26, 2017

monfera Apr 26, 2017

rreusser Apr 26, 2017 •

edited

rreusser Apr 26, 2017 •

edited

etpinard Apr 26, 2017

etpinard Apr 26, 2017

rreusser Apr 26, 2017 •

edited

alexcjohnson Apr 29, 2017

etpinard Apr 26, 2017

monfera Apr 26, 2017

etpinard Apr 26, 2017

monfera Apr 26, 2017

etpinard Apr 26, 2017

etpinard May 1, 2017 •

edited

monfera May 1, 2017

etpinard May 1, 2017

monfera May 1, 2017

etpinard May 1, 2017 •

edited

monfera May 1, 2017

etpinard commented May 9, 2017

monfera commented May 9, 2017

monfera commented May 9, 2017


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

Parcoords fonts #1624

Parcoords fonts #1624

Conversation

monfera commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Apr 26, 2017 • edited

Choose a reason for hiding this comment

rreusser Apr 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Apr 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented May 9, 2017

monfera commented May 9, 2017

monfera commented May 9, 2017

rreusser Apr 26, 2017 •

edited

rreusser Apr 26, 2017 •

edited

rreusser Apr 26, 2017 •

edited

etpinard May 1, 2017 •

edited

etpinard May 1, 2017 •

edited