Fix #1913 by pushing overflowed legend items to a new line

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃

[nathanjenx]

Fixes the handling of horizontal grouped legend items see #1913. Maintains the vertical grouping and includes gaps between traces (see below screenshot)

@antoinerg@etpinard

(edit: updated to show current behaviour, original available here)

[antoinerg]

Thank you @nathanjenx for submitting this PR 🎉

It seems like two mocks are not matching the baselines. Could you update the baselines by running:

$ npm run baseline legend_horizontal_groups
$ npm run baseline legendgroup_horizontal_wrapping

I will review the rest of your PR shortly 😸

[antoinerg]

About tracegroupgap, its description says that it

Sets the amount of vertical space (in px) between legend groups.

As of right now, it also affects the horizontal spacing. Could you get rid of this behaviour by not adding traceGroupGap to the widths that are computed?

cc @nathanjenx

[nathanjenx]

Removed traceGroupGap from all the width's & fixed up where the newline resets to (incase it's not 0) from:
groupXOffsets[groupXOffsets.length - 1] = 0;
to:
groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
@antoinerg

[antoinerg]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

[nathanjenx]

Thank you @nathanjenx for commit b9542c4 which removes tracegroupgap from horizontal margin.

I now realize however that we should still have some horizontal spacing between items to be in line with what we have when there are no grouped items. To do so, we should add a traceGap of 5 px to be equivalent to L672 of draw.js.

While you're at it, I think we should replace L672 from:
var traceGap = opts.tracegroupgap || 5; to var traceGap = 5;
Otherwise, tracegroupgap has an effect on legends without grouped items which shouldn't be the case.

Is commit 51178ec what you had in mind?

[archmoj]

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

[antoinerg]

@nathanjenx The horizontal spacing issue is still present.

If I remove all legendgroup from your mock, I get the following:

Now if I add a legendgroup to the first trace, I get:

I expected the horizontal spacing to remain the same 🤔

Concerning the changes in this baseline, I am wondering one should find a way to keep the axis line where it was before?

Indeed, it would be better if this baseline didn't change at all. @nathanjenx let me know if this is easy/possible!

Thank you again!

[nathanjenx]

@antoinerg the horizontal spacing was off because I was updating the width then checking it then adding it again, rather than checking if we would be too long + then adding the width if we aren't.

@antoinerg, @archmoj the axis line was off as I originally added traceGroupGap to EVERY row, not just new ones. The changes to the horizontal spacing of the legend items you see now are due to the traceGap being set to 5 (not the 10 it was inheriting from opts.traceGroupGap).

Should be all sorted now! 🎉

[antoinerg]

Thank you @nathanjenx for the thorough revision! It looks good to me 💃

[etpinard]

For comparison, this mock looks like:

on master.

[etpinard]

NON-BLOCKING

We could probably clean up these few lines a little:

• use for-loop here instead of forEach
• ⚡ that double empty new line
• use Lib.aggNums instead of Math.max.apply and group.reduce
• make the for(var i = 0, n = groupData.length; i < n; i++) loops for(var i = 0; i < groupData.length; i++), i.e. no need to define n (I think).

[antoinerg]

Done in a5fa247 except for group.reduce.

Can I really replace group.reduce with Lib.aggNums?

[etpinard]

Can I really replace group.reduce with Lib.aggNums?

Maybe with something like:

Lib.aggNums(function(a,b){returna+b[0].height;},0,groups)

similar to Lib.mean, but you're right, that's probably an overkill here as all the b[0].height should be defined. 💃

[etpinard]

@antoinerg Thanks for adding that test! I made one very non-blocking comment. Feel free to ignore it.

I'll let you the honours of merging this thing 💃 💃

Thank again @nathanjenx 🥇

[nathanjenx]

@etpinard@antoinerg sorry for going dark at the end there! Cheers for pushing it over the line! 💃💃