Hover label namelength #1822

ckiss · 2017-06-27T11:08:18Z

resolves #1777

ckiss · 2017-06-27T11:51:07Z

@etpinard as I said I'll do, I opened a new PR, with fixed tests. Let me know what you think. I still have some concerns that I might need to do some updates to the documentation or something. Not sure how that goes here :)

etpinard · 2017-06-27T14:14:35Z

src/components/fx/attributes.js

+            valType: 'number',
+            min: 0,
+            dflt: 15,
+            role: 'style',


We should try adding arrayOk support here, so that

trace = { x: [1, 2, 3], y: [2, 1, 2], namelength: [15, 10, 40] }

would work.

see #1761 for an example on how to proceed.

We should try adding arrayOk support here

good call - at first I was worried that this would just be confusing to look at ("I thought this was the same trace, why does it have a different name?") but it could actually come in handy for example if you vary the hover label font size, a bigger one may need more truncation of the name 👍

etpinard · 2017-06-27T14:16:05Z

src/components/fx/hover.js

@@ -689,7 +689,7 @@ function createHoverText(hoverData, opts, gd) {
            // strip out our pseudo-html elements from d.name (if it exists at all)
            name = svgTextUtils.plainText(d.name || '');

-            if(name.length > 15) name = name.substr(0, 12) + '...';
+            if(name.length > commonLabelOpts.namelength && commonLabelOpts.namelength > 0) name = name.substr(0, commonLabelOpts.namelength - 3) + '...';


Does this take care of namelength = 0 case which should hide the name text node completely?

++ can you rewrite this as

if(/* */) { }

@etpinard my understanding from this comment is that when namelength = 0 there should be no trimming happening, instead display the whole text... Am I wrong?

Too bad the JS token Infinity isn't JSON serializable. I'm ok with making namelength: 0 means no truncation, even though it feels contradictory to me.

Perhaps we should instead add support for an Infinity-like value in valType: 'number' attributes. For example, we could make namelength: null correspond to namelength === Infinity. Though, I can't think of any other attributes that could benefit from it, so I'm not sure it's worth the time.

cc @alexcjohnson

namelength: -1? In the style of indexOf, that says "does not apply" to me. Sort of.

@rreusser solid suggestion, I like it 👍

-1 sounds good for "no truncation." I'm not sure why someone would want to use namelength=0 to completely hide the name, rather than using hoverinfo but I bet someone will come up with a clever reason for it!

etpinard · 2017-07-14T21:10:44Z

@ckiss are you planning on addressing the comments above?

I might have time to wrap this one up on Monday.

ckiss · 2017-07-14T21:17:09Z

@etpinard I'll address them Monday morning! Have a great weekend and sorry for the delay!

- -1 for no constraint - more complete description - complete arrayOk support - tests

alexcjohnson · 2017-07-19T19:24:19Z

@ckiss I hope you don't mind, I took the liberty of finishing this up so we can get it in the next minor release. I switched to -1 for "no truncation", fleshed out the arrayOk support, and added a test of all these behaviors. Also, I realized that lengths 1-3 would have been meaningless as we had them, as it would be only ellipsis, so for those short lengths I omit the ellipsis and just show the leading characters. Sound reasonable?

@etpinard any comments?

etpinard · 2017-07-19T19:36:50Z

@etpinard any comments?

94f660a looks good 👍 . You'll need to add 'arrayOk' to this list to get the plot-schema tests to pass.

alexcjohnson · 2017-07-19T19:58:40Z

You'll need to add 'arrayOk' to this list to get the plot-schema tests to pass.

hah, first place we have an integer array, really? 😎 e937519

etpinard · 2017-07-19T20:01:56Z

💃

ckiss · 2017-07-19T20:05:27Z

src/components/fx/layout_attributes.js

-            valType: 'number',
-            min: 0,
+            valType: 'integer',
+            min: -1,
            dflt: 15,


don't you need to remove dflt here too?

I kept only the global default, because the trace-level default inherits from the global so doesn't use a default of its own.

You're right that we want a default - in fact, we need a default of 15 to preserve backward compatibility - this is just how that default gets propagated.

ckiss · 2017-07-19T20:07:49Z

@alexcjohnson @etpinard sorry I didn't had time to complete this :( I was back from holidays monday and had lots on my plate... thanks for your help @alexcjohnson. I added a comment to your commit. Otherwise I'm happy, although where this was initially discussed a default value was mentioned.

ckiss · 2017-07-19T20:25:06Z

cool, thanks!

ckiss changed the title ~~Hover label namelength~~ [WIP]Hover label namelength Jun 27, 2017

ckiss changed the title ~~[WIP]Hover label namelength~~ Hover label namelength Jun 27, 2017

ckiss force-pushed the hover-label-namelength branch from e939c86 to 542f39f Compare June 27, 2017 12:00

etpinard reviewed Jun 27, 2017

View reviewed changes

etpinard added status: has TODOs type: feature labels Jun 27, 2017

ckiss force-pushed the hover-label-namelength branch from 542f39f to c865049 Compare July 5, 2017 07:37

ckiss added 4 commits July 5, 2017 11:56

added hoverlabel namelength option

e7aeee4

zero means no trimming at all

8abf2ea

fixed tests

ac2f7f4

review fixes

8a308a8

ckiss force-pushed the hover-label-namelength branch from c865049 to 8a308a8 Compare July 5, 2017 08:59

finish and test hoverlabel.namelength

94f660a

- -1 for no constraint - more complete description - complete arrayOk support - tests

etpinard added this to the 1.29.0 milestone Jul 19, 2017

etpinard added status: reviewable and removed status: has TODOs labels Jul 19, 2017

integer can be arrayOk

e937519

ckiss commented Jul 19, 2017

View reviewed changes

alexcjohnson merged commit bc5726c into plotly:master Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hover label namelength #1822

Hover label namelength #1822

ckiss commented Jun 27, 2017 •

edited by etpinard

Loading

ckiss commented Jun 27, 2017

etpinard Jun 27, 2017

etpinard Jun 27, 2017

alexcjohnson Jun 28, 2017

etpinard Jun 27, 2017 •

edited

Loading

etpinard Jun 27, 2017

ckiss Jun 28, 2017

etpinard Jun 28, 2017

rreusser Jun 28, 2017

etpinard Jun 28, 2017

alexcjohnson Jun 28, 2017

etpinard commented Jul 14, 2017

ckiss commented Jul 14, 2017

alexcjohnson commented Jul 19, 2017

etpinard commented Jul 19, 2017 •

edited

Loading

alexcjohnson commented Jul 19, 2017

etpinard commented Jul 19, 2017

ckiss Jul 19, 2017

alexcjohnson Jul 19, 2017

ckiss commented Jul 19, 2017

ckiss commented Jul 19, 2017

Hover label namelength #1822

Hover label namelength #1822

Conversation

ckiss commented Jun 27, 2017 • edited by etpinard Loading

ckiss commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jun 27, 2017 • edited Loading

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 commented Jul 14, 2017

ckiss commented Jul 14, 2017

alexcjohnson commented Jul 19, 2017

etpinard commented Jul 19, 2017 • edited Loading

alexcjohnson commented Jul 19, 2017

etpinard commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckiss commented Jul 19, 2017

ckiss commented Jul 19, 2017

ckiss commented Jun 27, 2017 •

edited by etpinard

Loading

etpinard Jun 27, 2017 •

edited

Loading

etpinard commented Jul 19, 2017 •

edited

Loading