Break up legend module #329

etpinard · 2016-03-14T19:05:01Z

@mdtusz - in preparation for range selectors.

This PR:

replaces the nested legend <svg> by a <g clip-path="">
breaks up the legend defaults, draw and style step into separate files.
adds a helpers module
adds an anchor_utils module (to handle e.g. the xanchor: 'auto' positioning case) which will be reuse in the range selector module.
adds tests for the legend helpers and anchor_utils
adds in-house getBBox test asset (which considers the clip-path)

- un-exposed legend.lines, legend.bars, legend.points, legend.boxes and legend.pie

- un-expose legend.repositionText and legend.texts - use anchor utils

etpinard · 2016-03-14T19:07:16Z

src/components/legend/style.js

+};
+
+function styleLines(d) {
+    var trace = d[0].trace,


these should really be methods of the corresponding trace module, but that will be for a future PR.

- the exit selection will always be empty as the joined data is always [0].

- append <defs> to toppaper in make-framework step - use fullLayout._uid in legend <clipPath> id to avoid conflict on DOM queries.

- needed to determine bounding box of clipped element, as the stock SVG getBBox method does not take clip path into consideration - use it legend scroll test to determine the height of the legend

- to ensure that they are properly removed when 'showlegend' is relayout'ed to false.

- sub-pixel diff due to legend clipping

etpinard · 2016-03-15T20:45:19Z

src/plot_api/plot_api.js

@@ -2585,6 +2585,9 @@ function makePlotFramework(gd) {
    fullLayout._defs = fullLayout._paper.append('defs')
        .attr('id', 'defs-' + fullLayout._uid);

+    fullLayout._topdefs = fullLayout._toppaper.append('defs')
+        .attr('id', 'defs-' + fullLayout._uid);


@mdtusz I opted for a similar pattern for <defs> in the top paper <svg> as is currently done in the (bottom) paper <svg>

Then the clipPath should remain positioned relative to the element it's applied to?

After a few quick Google searches, I didn't find anything arguments for or against relative clipPath locations.

I think that having one <defs> node per <svg> does make things a bit clearer. But, above all, I chose this route for consistency with what's currently done in the (bottom) <svg>.

- getBBox fails when call on a clipPath node in FF - use DOM attributes as fallback to grab the bounding box

etpinard · 2016-03-16T13:30:24Z

test/jasmine/assets/get_bbox.js

+
+    try {
+        // this line throws an error in FF (38 and 45 at least)
+        clipBBox = clipPath.node().getBBox();


@mdtusz are you ok with this?

mdtusz · 2016-03-16T16:45:02Z

Ok I think this looks pretty good to go after the few minor little nits.

💃

- that way ids remain unique on page - rm special <defs> merge block in to-svg step

- as childNodes prop gets mutated in top-group loop

etpinard · 2016-03-16T17:46:23Z

src/plot_api/plot_api.js

@@ -2586,7 +2586,7 @@ function makePlotFramework(gd) {
        .attr('id', 'defs-' + fullLayout._uid);

    fullLayout._topdefs = fullLayout._toppaper.append('defs')
-        .attr('id', 'defs-' + fullLayout._uid);
+        .attr('id', 'topdefs-' + fullLayout._uid);


FYI @mdtusz I changed my mind.

We should make sure that our ids are unique on the page at all times.

svg ids are treated as regular DOM ids. They are not scoped to one particular <svg>:

http://codepen.io/etpinard/pen/ONWVaL

- use it in legend text - rm now useless user-select purge step in to-svg

etpinard · 2016-03-16T18:35:58Z

src/components/legend/draw.js

-        'user-select': 'none'
-    })
+    .style('text-anchor', 'start')
+    .classed('user-select-none', true)


@mdtusz cleaner, right?

I'm going to use .user-select-none in the range selector buttons.

Ah yep much better. We can add that to axes values as well - when sliding the range-slider they sometimes get highlighted.

Break up legend module

etpinard added 7 commits March 14, 2016 14:52

factor out legend helper into separate file + add tests

a40fc83

move auto-anchor logic to own module + add tests

b3af445

move legend defaults to own module

67eda75

move legend style step to own module:

09354bf

- un-exposed legend.lines, legend.bars, legend.points, legend.boxes and legend.pie

move legend-data logic to own module

1b69395

move legend draw step to own module:

0b6dfcf

- un-expose legend.repositionText and legend.texts - use anchor utils

lint

aa13bfb

etpinard added the type: maintenance label Mar 14, 2016

etpinard reviewed Mar 14, 2016
View reviewed changes

etpinard added 10 commits March 15, 2016 12:41

fix typo (function name)

dcea0dd

lint (mostly infix spacing)

a11e698

make legend element a <g> instead of nestead <svg>

cdd7240

cropped legend element using clip-path on legend <g>

7b393d4

rm useless .exit().remove() :

a7829cf

- the exit selection will always be empty as the joined data is always [0].

robustify top-paper <defs>:

59b0c52

- append <defs> to toppaper in make-framework step - use fullLayout._uid in legend <clipPath> id to avoid conflict on DOM queries.

merge top-paper <defs> into main svg on to-svg step

0007807

add in-house getBBox test asset:

335bb40

- needed to determine bounding box of clipped element, as the stock SVG getBBox method does not take clip path into consideration - use it legend scroll test to determine the height of the legend

add legend and clip path count test,

857878b

- to ensure that they are properly removed when 'showlegend' is relayout'ed to false.

update baseline:

dcd0cb5

- sub-pixel diff due to legend clipping

etpinard reviewed Mar 15, 2016
View reviewed changes

add fallback for FF in getBBox test asset:

8440ef1

- getBBox fails when call on a clipPath node in FF - use DOM attributes as fallback to grab the bounding box

etpinard added the status: reviewable label Mar 15, 2016

etpinard reviewed Mar 16, 2016
View reviewed changes

lint (define variable in block)

757ffdf

etpinard added 2 commits March 16, 2016 13:43

change top-paper <defs> id to 'topdefs' + uid:

196d7dc

- that way ids remain unique on page - rm special <defs> merge block in to-svg step

make copy of top-paper childNodes array-like:

860e647

- as childNodes prop gets mutated in top-group loop

etpinard reviewed Mar 16, 2016
View reviewed changes

make user-select-none CSS class:

dbaf340

- use it in legend text - rm now useless user-select purge step in to-svg

etpinard reviewed Mar 16, 2016
View reviewed changes

etpinard added a commit that referenced this pull request Mar 16, 2016

Merge pull request #329 from plotly/break-up-legend

a27d62c

Break up legend module

etpinard merged commit a27d62c into master Mar 16, 2016

etpinard deleted the break-up-legend branch March 16, 2016 18:54

etpinard mentioned this pull request Mar 17, 2016

Feature range slider #336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Break up legend module #329

Break up legend module #329

Uh oh!

etpinard commented Mar 14, 2016

Uh oh!

etpinard Mar 14, 2016

Uh oh!

etpinard Mar 15, 2016

Uh oh!

etpinard Mar 16, 2016

Uh oh!

mdtusz commented Mar 16, 2016

Uh oh!

etpinard Mar 16, 2016

Uh oh!

etpinard Mar 16, 2016

Uh oh!

etpinard Mar 16, 2016

Uh oh!

mdtusz Mar 16, 2016

Uh oh!

Uh oh!

Uh oh!

Break up legend module #329

Break up legend module #329

Uh oh!

Conversation

etpinard commented Mar 14, 2016

Uh oh!

etpinard Mar 14, 2016

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 15, 2016

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

mdtusz commented Mar 16, 2016

Uh oh!

etpinard Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

mdtusz Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

Uh oh!