[nvd3] refactor, thinner margins #6282
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6282 +/- ##
=========================================
+ Coverage 76.91% 77.01% +0.1%
=========================================
Files 64 64
Lines 9508 9508
=========================================
+ Hits 7313 7323 +10
+ Misses 2195 2185 -10
Continue to review full report at Codecov.
|
| const maxXAxisLabelHeight = getMaxLabelSize(svg, 'nv-x'); | ||
| margins.left = maxYAxisLabelWidth + marginPad; | ||
|
|
||
| if (yAxisLabel && yAxisLabel !== '') { |
There was a problem hiding this comment.
nit -- the second condition will never be reached because '' is falsy
There was a problem hiding this comment.
I copied this blindly from line 595 but happy to improve it.
| // Hack to adjust margins to accommodate long axis tick labels. | ||
| // - has to be done only after the chart has been rendered once | ||
| // - measure the width or height of the labels | ||
| // ---- (x axis labels are rotated 45 degrees so we use height), | ||
| // - adjust margins based on these measures and render again | ||
| const margins = chart.margin(); | ||
| margins.bottom = 28; | ||
| if (chart.xAxis) { | ||
| margins.bottom = 28; |
There was a problem hiding this comment.
just to understand -- we are modifying the reference to the chart margin object directly? should we instead copy it and call chart.margin(newMargin)? if it works I guess that's fine.
There was a problem hiding this comment.
Right. I understand how non-functional that is and didn't to go too deep in the refactor. I mostly just grouped all margin-related code in one section and made sure that logically would produce the same results. Hopefully we can get rid of this whole module and nvd3 fairly soon!
|
Hey since I got the approval and wanted to minimize the changes as much as possible, I'll just merge this as-is. |
|
sg 👍 |
mistercrunch
added
🏷️ bot
There was an unneeded margin on the pie chart + cleaned up the margin-related code a bit.