-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Adjusted top margin of heatmap plot to get it working in V2 #1361
Conversation
Problem: The heatmap in V2 was shifted towards the top margin of slice container, this was because in v2 slice name header was part of the container body, while in v1 the header was separately defined in explore.html template. Solution: To get heatmap properly shown in V2, we need to take into account the height of the slice_name header. Adding to margin_top will shift the plot in V1 too, but it won't make a big difference to the look. Ideally when we renovate slice container in future PR we would defined a height for slice_name header and take it into account for all visualization files.
@@ -38,7 +38,7 @@ class ChartContainer extends React.Component { | |||
|
|||
width: () => this.chartContainerRef.getBoundingClientRect().width, | |||
|
|||
height: () => parseInt(this.props.height, 10) - 100, | |||
height: () => parseInt(this.props.height, 10), |
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.
I'm not sure why we do -100 here previously, the heatmap was squashed in y direction with -100 in height.
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.
- 100
here was because the bottom of some charts were being cut off. probably a better way to handle this is in the margin declarations for each chart, or in the component more specifically.
can you check that the other charts are rendered correctly with this change?
you make a good point here:
can we handle this in the current |
Instead of hardcoding a margin top size, I added header_height in margin_top for heatmap.js @ascott |
@@ -10,8 +10,11 @@ require('./heatmap.css'); | |||
// https://jsfiddle.net/cyril123/h0reyumq/ | |||
function heatmapVis(slice) { | |||
function refresh() { | |||
// Header for panel in explore v2 | |||
const header = document.getElementsByClassName('panel-title'); | |||
const headerHeight = header ? header[0].offsetHeight : 0; |
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.
i would use getBoundingClientRect
here, it has better browser support compared to offsetHeight
https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetHeight
can you confirm that this change doesn't affect the current explore view too much?
3c62dd5
to
0b147f7
Compare
@@ -10,8 +10,11 @@ require('./heatmap.css'); | |||
// https://jsfiddle.net/cyril123/h0reyumq/ | |||
function heatmapVis(slice) { | |||
function refresh() { | |||
// Header for panel in explore v2 | |||
const header = document.getElementsByClassName('panel-title'); |
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.
sorry one more comment, are we sure there will only be one panel-title
class on the page? will it be the first one?
5d78515
to
82124df
Compare
82124df
to
a62d9ad
Compare
LGTM 🚢 thanks @vera-liu! |
Good point! Added id to the header of slice in latest commit:) On Mon, Oct 24, 2016 at 9:32 PM, Alanna Scott notifications@github.com
Best, |
Problem:
The heatmap in V2 was shifted towards the top margin of slice
container, this was because in v2 slice name header was part of the
container body, while in v1 the header was separately defined in
explore.html template.
Solution:
To get heatmap properly shown in V2, we need to
take into account the height of the slice_name header. Adding to
margin_top will shift the plot in V1 too, but it won't make a big
difference to the look.
Ideally when we renovate slice container in future PR we would defined a
height for slice_name header and take it into account for all
visualization files.
Before (v2):
After (v2):
After (v1):
@ascott