pissang commented on a change in pull request #16493:
URL: https://github.com/apache/echarts/pull/16493#discussion_r805507967
##########
File path: src/chart/heatmap/HeatmapView.ts
##########
@@ -225,6 +228,17 @@ class HeatmapView extends ChartView {
for (let idx = start; idx < end; idx++) {
let rect;
const style = data.getItemVisual(idx, 'style');
+ const itemModel = data.getItemModel<HeatmapDataItemOption>(idx);
+
+ // borderRadius must be set before the Rect draw.
Review comment:
It's not necessary to fetch the borderRadius before creating `Rect`. You
can still change the shape after `Rect` created. I will describe it bellow.
##########
File path: src/chart/heatmap/HeatmapView.ts
##########
@@ -245,13 +259,19 @@ class HeatmapView extends ChartView {
dataDimY
]);
+ const shape: Shape = {
+ x: Math.floor(Math.round(point[0]) - width / 2),
+ y: Math.floor(Math.round(point[1]) - height / 2),
+ width: Math.ceil(width),
+ height: Math.ceil(height)
+ };
+
+ if (borderRadius || Array.isArray(borderRadius)) {
Review comment:
As described above. The `r` can still be changed after `Rect` created.
So it's not necessary to prepare the `shape` with `r` in two different
branches. You only need to use
```
rect.shape.r = borderRadius;
```
to update the `r` around the line
https://github.com/apache/echarts/blob/04392f5336f0f895c37b3e3f36294b67fc5fdf5c/src/chart/heatmap/HeatmapView.ts#L326
##########
File path: src/chart/heatmap/HeatmapView.ts
##########
@@ -41,6 +41,8 @@ interface GeoLikeCoordSys extends CoordinateSystem {
getViewRect(): graphic.BoundingRect
}
+type Shape = graphic.Rect['shape'];
Review comment:
It's better to be named `RectShape`
##########
File path: src/chart/heatmap/HeatmapView.ts
##########
@@ -245,13 +259,19 @@ class HeatmapView extends ChartView {
dataDimY
]);
+ const shape: Shape = {
+ x: Math.floor(Math.round(point[0]) - width / 2),
+ y: Math.floor(Math.round(point[1]) - height / 2),
+ width: Math.ceil(width),
+ height: Math.ceil(height)
+ };
+
+ if (borderRadius || Array.isArray(borderRadius)) {
Review comment:
Also using `isArray` method in the `zrender/src/core/util` instead of
`Array.isArray`.
In fact the `Array.isArray` check can be removed because we usually don't do
strict type checking in the logic code. Usually we only do the check that may
have exceptions and break the whole app.
For example in this case `borderRadius` we won't reject the value like `{ 0:
5, 1: 5, 2: 5, 3: 5 }` or `TypedArray`. It make users code can still run even
if they made a small mistake.
Instead we are trying to leave this kind of checking to the TypeScript.
Which can provide strict enough type check and won't bring any extra code to
the lib.
##########
File path: test/heatmap-border-radius.html
##########
@@ -0,0 +1,166 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+'License'); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<html>
+
+<head>
+ <meta charset='utf-8'>
+ <script src='lib/simpleRequire.js'></script>
+ <script src='lib/config.js'></script>
+ <meta name='viewport' content='width=device-width, initial-scale=1' />
+</head>
+
+<body>
+ <style>
+ html,
+ body,
+ #main,
+ #main2 {
+ width: 100%;
+ height: 100%;
+ margin: 0;
+ }
+ </style>
+ <div id='main'></div>
+ <div id='main2'></div>
+ <script>
+
+ require([
+ 'echarts'
+ ], function (echarts) {
+
+ var chart = echarts.init(document.getElementById('main'));
+ var chartTwo = echarts.init(document.getElementById('main2'));
+
+ var hours = ['12a', '1a', '2a', '3a', '4a', '5a', '6a',
Review comment:
Try using `npm run mktest` command to create a new test case and put all
heatmap borderRadius related test cases in one file.
##########
File path: test/heatmap-calendar.html
##########
@@ -0,0 +1,106 @@
+<!DOCTYPE html>
+<!--
Review comment:
There is already a `calendar-heatmap` case. Add a new `heatmap-calendar`
may be confusing
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
