You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2020/07/19 05:00:16 UTC
[GitHub] [incubator-echarts] quillblue opened a new pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
quillblue opened a new pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [X] bug fixing
- [ ] new feature
- [ ] others
### 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
<!--
- #xxxx: ...
-->
#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 to `xAxis`. 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?
- [ ] The API has been changed.
<!-- LIST THE API CHANGES HERE -->
### Related test cases or examples to use the new APIs
NA.
## Others
### Merging options
- [ ] Please squash the commits into a single one when merge.
### 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] quillblue commented on a change in pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
quillblue commented on a change in pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#discussion_r456868341
##########
File path: test/heatmap.html
##########
@@ -19,91 +18,156 @@
-->
<html>
- <head>
Review comment:
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#issuecomment-661607152
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#issuecomment-661607114
LGTM now
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] quillblue commented on a change in pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
quillblue commented on a change in pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#discussion_r456868773
##########
File path: src/chart/heatmap/HeatmapView.ts
##########
@@ -203,8 +206,8 @@ class HeatmapView extends ChartView {
let rect;
if (isCoordinateSystemType<Cartesian2D>(coordSys, 'cartesian2d')) {
- // 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;
Review comment:
I agree, have modified accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#discussion_r456867222
##########
File path: test/heatmap.html
##########
@@ -19,91 +18,156 @@
-->
<html>
- <head>
Review comment:
Should create a new test case instead of modifying the exists.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang merged pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#discussion_r456867222
##########
File path: test/heatmap.html
##########
@@ -19,91 +18,156 @@
-->
<html>
- <head>
Review comment:
Should create a new test case instead of modifying the exists cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#discussion_r456867080
##########
File path: src/chart/heatmap/HeatmapView.ts
##########
@@ -203,8 +206,8 @@ class HeatmapView extends ChartView {
let rect;
if (isCoordinateSystemType<Cartesian2D>(coordSys, 'cartesian2d')) {
- // 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;
Review comment:
data.get(dataDims[0], idx) is better to be assigned to a temporal variable.
Also, yAxis should also be checked?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #12991: fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969)
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12991:
URL: https://github.com/apache/incubator-echarts/pull/12991#issuecomment-660587762
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/incubator-echarts/wiki/How-to-make-a-pull-request).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org