-
Notifications
You must be signed in to change notification settings - Fork 19.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
fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969) #12991
Conversation
Thanks for your contribution! |
// Ignore empty data | ||
if (isNaN(data.get(dataDims[2], idx) as number)) { | ||
// Ignore empty data and out of extent data | ||
if (isNaN(data.get(dataDims[2], idx) as number) || data.get(dataDims[0], idx) < xAxisExtent[0] || data.get(dataDims[0], idx) > xAxisExtent[1]) { | ||
continue; |
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.
data.get(dataDims[0], idx) is better to be assigned to a temporal variable.
Also, yAxis should also be checked?
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 agree, have modified accordingly.
@@ -19,91 +18,156 @@ | |||
--> | |||
|
|||
<html> | |||
<head> |
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.
Should create a new test case instead of modifying the exists.
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 the new case is almost seprate except under same page and share same data. Changes are more likely a false detection (may be due to formatter, actually I didn't change the original case any)
LGTM now |
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
Brief Information
This pull request is in the type of:
What does this PR do?
While rendering data for heatmap, judgement on whether data item will be out of current axis scale extent were implemented, and those items out of visible range will be skiped.
Fixed issues
#12969
Details
Before: What was the problem?
As original issue states, yAxis labels will be covered by color blocks (data series) in heatmap when
min
option was applied toxAxis
. Actually these blocks should not be rendered since they are not in current extent of xAxis.After: How is it fixed in this PR?
A judgement before rendering rect for each data item was added to calculate whether it will be in the extent, and rendering will be skipped if it is not in the extent.
This change also passed tests for scale changes cases such as using
dataZoom
control, using data zoom in toolbox, etc.Usage
Are there any API changes?
Related test cases or examples to use the new APIs
NA.
Others
Merging options
Other information
I have to admit that it's not a so perfect solution. A general solution for handling this case (judge whether dataItem is in the scale of current displayed coord) will sure be better but failed to find one, since I found that different series handle such kind of cases in different ways and hard to find a general one also applicable to heatmap. Feel free to comment if have any better idea.