Fix legend scrollbox and border overlap #474
Conversation
* Moved clip-path from legend to scrollbox, to ensure the scrollbox doesn't overlap with the legend border. * Some jasmine tests failed because they used the legend clip-path to determine the legend height. Added attribute `data-height` to legend to help these jasmine tests. * The baseline image for `legend_scroll.json` needed updating because the scrollbox in that mock overlapped with the legend border.
| scrollBox = createScrollBox(), // contains all the legend traces | ||
| scrollBar = createScrollBar(), // scroll bar (visible only if needed) | ||
| groups = createGroups(), // legend groups | ||
| traces = createLegendTraces(); // legend traces |
There was a problem hiding this comment.
I'm not a fan of this pattern.
I'm more in the school of don't make a bunch of 10-line functions if you're only going to use them once.
@mdtusz this is mostly your code, what are your thoughts on this?
There was a problem hiding this comment.
Agreed. Small functions are great, but only if they're generalised - otherwise it's just more mental overhead to jump to them (although I'm guilty of doing this in some places too).
There was a problem hiding this comment.
: | I already did something similar here. The main purpose is to document the code.
Shall I revert this change and avoid it in the future?
There was a problem hiding this comment.
@n-riesco the difference between c53303b and the ⏫ is that the small functions in the shape code are called inside if-else. Spitting the lifecycle steps into separate functions in that case was valuable (who wants to read a 20+ line else block?).
In the above ⏫, there's no if-else clauses, just node update patterns.
There was a problem hiding this comment.
This PR contains:
legend.drawThis PR is ready for review. Next I'm planning to address issue #53 .