-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(chart & heatmap): make to fix that y label is rendering out of bounds #20011
fix(chart & heatmap): make to fix that y label is rendering out of bounds #20011
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20011 +/- ##
==========================================
- Coverage 66.28% 66.28% -0.01%
==========================================
Files 1712 1712
Lines 63968 63969 +1
Branches 6731 6731
==========================================
Hits 42404 42404
- Misses 19853 19854 +1
Partials 1711 1711
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -121,6 +121,7 @@ function Heatmap(element, props) { | |||
let longestY = 1; | |||
|
|||
records.forEach(datum => { | |||
if (typeof datum.y === 'number') pixelsPerCharY = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the user change the font size? Will it work in that case?
Please add a comment above explaining why this is adjusted like so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soon, we may be dealing with different fonts, tool. Is this an issue with the ECharts library itself that we're just working around here? If so, I wonder if they've already addressed it in a newer version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user can't change the font size of y label.
I think that this maybe the issue of legacy charts library itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually It is implemented using d3 and the implementation does not seem elegant. Let's migrate to echarts later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will later, but we need to get that prioritized on the roadmap... it'll be a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on my end.. @rusackas @stephenLYZ in your court if you want to deal with fonts now or punt it, since that's a larger discussion and this is a legacy plugin
/testenv up |
@rusackas Ephemeral environment spinning up at http://52.27.240.250:8080. Credentials are |
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
[viz] heatmap Y axis labels rendering out-of-bounds
Description
Heatmap Y axis label rendering out-of-bounds, despite using
auto
for theLEFT MARGIN
control.This issue is happened when the Y axis value is number type and in case of that Y axis value is string type, Y axis label is rendering correctly in bounds. So I fixed this issue by increasing the pixel value per char in case of number type.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
heatmap-out-bound.mov
AFTER:
fix-heatmap-outbounds.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION