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/09 15:28:44 UTC
[GitHub] [incubator-echarts] susiwen8 opened a new pull request #12947: Tooltip
susiwen8 opened a new pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [ ] bug fixing
- [ ] new feature
- [x] others
### What does this PR do?
New tooltip style for 5.0
### Fixed issues
Close https://github.com/apache/incubator-echarts/issues/12929
## Details
### Before: What was the problem?
<!-- DESCRIBE THE BUG OR REQUIREMENT HERE. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
### After: How is it fixed in this PR?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
## 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
- [x] Please squash the commits into a single one when merge.
### Other information
----------------------------------------------------------------
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] 100pah commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
100pah commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r457857968
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -259,16 +314,24 @@ class TooltipHTMLContent {
// this.hide();
}
- show(tooltipModel: Model<TooltipOption>) {
+ show(tooltipModel: Model<TooltipOption>, nearPointColor: ZRColor) {
clearTimeout(this._hideTimeout);
const el = this.el;
const styleCoord = this._styleCoord;
+ const offset = el.offsetHeight / 2;
+ if (zrUtil.isObject(nearPointColor) && nearPointColor.type !== 'pattern') {
+ nearPointColor = nearPointColor.colorStops[0].color;
+ }
+ else if (zrUtil.isObject(nearPointColor) && (nearPointColor.type === 'pattern')) {
+ nearPointColor = 'transparent';
+ }
el.style.cssText = gCssText + assembleCssText(tooltipModel)
// Because of the reason described in:
// http://stackoverflow.com/questions/21125587/css3-transition-not-working-in-chrome-anymore
// we should set initial value to `left` and `top`.
- + ';left:' + styleCoord[0] + 'px;top:' + styleCoord[1] + 'px;'
+ + ';left:' + styleCoord[0] + 'px;top:' + (styleCoord[1] - offset) + 'px;'
+ + `border-color: ${nearPointColor}`
Review comment:
Should be `border-color: ${nearPointColor};`, missing a `;`.
Otherwise, the following `extraCssText` will be broken.
##########
File path: src/component/axisPointer/findPointFromSeries.ts
##########
@@ -57,7 +58,20 @@ export default function (finder: {
const el = data.getItemGraphicEl(dataIndex);
const coordSys = seriesModel.coordinateSystem;
- if (seriesModel.getTooltipPosition) {
+ if (coordSys && coordSys.dataToPoint && finder.isStacked) {
+ const baseAxis = coordSys.getBaseAxis();
+ const valueAxis = coordSys.getOtherAxis(baseAxis as any);
+ const valueAxisDim = valueAxis.dim;
+ const baseAxisDim = baseAxis.dim;
+ const baseDataOffset = valueAxisDim === 'x' || valueAxisDim === 'radius' ? 1 : 0;
+ const baseDim = data.mapDimension(baseAxisDim);
+ const stackedData = [];
+ stackedData[baseDataOffset] = data.get(baseDim, dataIndex);
+ stackedData[1 - baseDataOffset] = data.get(data.getCalculationInfo('stackResultDimension'), dataIndex);
+ point = coordSys.dataToPoint(stackedData) || [];
+ console.log(11111111)
Review comment:
`console.log(11111111)` should not exist.
##########
File path: src/component/axisPointer/findPointFromSeries.ts
##########
@@ -57,7 +58,20 @@ export default function (finder: {
const el = data.getItemGraphicEl(dataIndex);
const coordSys = seriesModel.coordinateSystem;
- if (seriesModel.getTooltipPosition) {
+ if (coordSys && coordSys.dataToPoint && finder.isStacked) {
Review comment:
In my opinion, `seriesModel.getTooltipPosition` should have higher priority than this logic.
And this logic is similar logic as the `else if` branch in line 77~85., so this code can be put there.
##########
File path: src/component/marker/MarkerModel.ts
##########
@@ -206,19 +203,16 @@ abstract class MarkerModel<Opts extends MarkerOption = MarkerOption> extends Com
const value = this.getRawValue(dataIndex);
const formattedValue = zrUtil.isArray(value)
? zrUtil.map(value, addCommas).join(', ') : addCommas(value as number);
- const name = data.getName(dataIndex);
+ const name = encodeHTML(data.getName(dataIndex));
Review comment:
If `name` is encoded here, then it will be added to var `html`, and then it will be input to `concatTooltipHtml` and be encoded twice. That would be wrong if there are some char like '<' ';' '>' in name.
##########
File path: src/component/marker/markerHelper.ts
##########
@@ -81,9 +81,9 @@ function markerTypeCalculatorWithExtent(
? data.getCalculationInfo('stackResultDimension')
: targetDataDim;
- const value = numCalculate(data, calcDataDim, markerType);
+ const value = numCalculate(data, targetDataDim, markerType);
Review comment:
Not sure why make this change. It's seems not correct for stacked chart?
##########
File path: src/chart/radar/RadarSeries.ts
##########
@@ -101,8 +101,10 @@ class RadarSeriesModel extends SeriesModel<RadarSeriesOption> {
return encodeHTML(name === '' ? this.name : name) + '<br/>'
+ zrUtil.map(indicatorAxes, function (axis, idx) {
const val = data.get(data.mapDimension(axis.dim), dataIndex);
- return encodeHTML(axis.name + ' : ' + val);
- }).join('<br />');
+ return '<p style="margin: 8px 0 0;">'
Review comment:
Here we use `<p>` instead of the previous `<br/>`. In my opinion, may be we should add an inline css 'display: block' to prevent the `display` of `<p>` from being overridden by the environment.
(I know it not appropriate to do that override in most cases, but env is harsh and we can do something to reduce the bad cases if possible.)
in my opinion, do not use <p>, in case that p is defined other css properties like `line-height`, padding, and so on.
It's OK to use div.
##########
File path: src/component/tooltip/TooltipModel.ts
##########
@@ -63,6 +63,8 @@ export interface TooltipOption extends CommonTooltipOption<TopLevelFormatterPara
* Only available when renderMode is html
*/
appendToBody?: boolean
+
+ order?: 'valueAsc' | 'valueDesc' | 'legendAsc' | 'legendDesc'
Review comment:
(1) Can not find the test case about this feature in `new-tooltip.html`?
(2) I think it should not be named as 'legendDesc'.
It is not the legend order if legend.data declared in an other order.
In fact it is the "series declaration order".
(3) Do we need the config that "order by series declaration but desc" ?
I've not found a use case for that yet.
And do we need the config that "order by value but inverse to the display"? That will be a miss leading and probably never be used, I guess.
So I think here really meaningful options would be only:
+ order by series declaration (by default)
+ order by values on axis (the order should be the same as they are displayed, that is, if `yAxis.inverse: true` is specified, the order should be followed.
What's your opinion @pissang @Ovilia ?
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -38,6 +39,55 @@ const vendors = ['', '-webkit-', '-moz-', '-o-'];
const gCssText = 'position:absolute;display:block;border-style:solid;white-space:nowrap;z-index:9999999;';
+function mirrowPos(pos: string): string {
+ pos = pos === 'left'
+ ? 'right'
+ : pos === 'right'
+ ? 'left'
+ : pos === 'top'
+ ? 'bottom'
+ : 'top';
+ return pos;
+}
+
+function assembleArrow(
+ backgroundColor: ColorString,
+ borderColor: ZRColor,
+ arrowPosition: TooltipOption['position']
+) {
+ if (zrUtil.isObject(borderColor) && borderColor.type !== 'pattern') {
Review comment:
Probably better to directly detect the feature of "gradient color" rather than `!== 'pattern'`?
For example: detect whether `colorStop` exist?
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -79,6 +129,7 @@ function assembleCssText(tooltipModel: Model<TooltipOption>) {
const transitionDuration = tooltipModel.get('transitionDuration');
const backgroundColor = tooltipModel.get('backgroundColor');
+ const boxShadow = tooltipModel.get('boxShadow');
Review comment:
There is no echarts option named as `boxShadow` yet but have lots of option named as `shadowColor` `shadowBlur` `shadowOffsetX` `shadowOffsetY`.
I think we should better follow that convention, to give users consistent knowledge.
Especially there is another type of tooltip implemented not depends on DOM (for mini program env). The new `option` box shadow is a CSS value, which brings trouble to parse if we intent to adopt shadow feature in non-DOM tooltip.
Probably we can retrieve `shadowColor` `shadowBlur` `shadowOffsetX` `shadowOffsetY` from tooltipModel and make css `box-shadow` by these values.
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -259,16 +314,24 @@ class TooltipHTMLContent {
// this.hide();
}
- show(tooltipModel: Model<TooltipOption>) {
+ show(tooltipModel: Model<TooltipOption>, nearPointColor: ZRColor) {
clearTimeout(this._hideTimeout);
const el = this.el;
const styleCoord = this._styleCoord;
+ const offset = el.offsetHeight / 2;
+ if (zrUtil.isObject(nearPointColor) && nearPointColor.type !== 'pattern') {
Review comment:
This code snippet is repeated with above code in line 58, should better be put in an util function.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -735,14 +782,20 @@ class TooltipView extends ComponentView {
const formatter = tooltipModel.get('formatter');
positionExpr = positionExpr || tooltipModel.get('position');
let html = defaultHtml;
+ const nearPoint = this._getNearestPoint(
+ [x, y],
+ params,
+ (tooltipModel.ecModel.getComponent('series') as CoordinateSystemHostModel)
Review comment:
Should not get the coordinate system from the "first" series.
A chart instance can contain multiple series and the first series might do not belong to this axis or even do not belong to this coordinate system.
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -38,6 +39,55 @@ const vendors = ['', '-webkit-', '-moz-', '-o-'];
const gCssText = 'position:absolute;display:block;border-style:solid;white-space:nowrap;z-index:9999999;';
+function mirrowPos(pos: string): string {
+ pos = pos === 'left'
+ ? 'right'
+ : pos === 'right'
+ ? 'left'
+ : pos === 'top'
+ ? 'bottom'
+ : 'top';
+ return pos;
+}
+
+function assembleArrow(
+ backgroundColor: ColorString,
+ borderColor: ZRColor,
+ arrowPosition: TooltipOption['position']
+) {
+ if (zrUtil.isObject(borderColor) && borderColor.type !== 'pattern') {
+ borderColor = borderColor.colorStops[0].color;
+ }
+ else if (zrUtil.isObject(borderColor) && (borderColor.type === 'pattern')) {
+ borderColor = 'transparent';
+ }
+ if (typeof arrowPosition !== 'string') {
Review comment:
`zrUtil.isString(arrowPosition)` is more neat.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -541,23 +565,47 @@ class TooltipView extends ComponentView {
renderMode
});
- if (dataParams) {
- singleParamsList.push(dataParams);
- const seriesTooltip = series.formatTooltip(
- dataIndex, true, null, renderMode as TooltipRenderMode
- );
-
- let html;
- if (zrUtil.isObject(seriesTooltip)) {
- html = seriesTooltip.html;
- const newMarkers = seriesTooltip.markers;
- zrUtil.merge(markers, newMarkers);
- }
- else {
- html = seriesTooltip;
- }
- seriesDefaultHTML.push(html);
+ singleParamsList.push(dataParams);
+ const seriesTooltip = series.formatTooltip(
+ dataIndex, true, null, renderMode as TooltipRenderMode
+ );
+
+ let html;
+ if (zrUtil.isObject(seriesTooltip)) {
+ html = seriesTooltip.html;
+ const newMarkers = seriesTooltip.markers;
+ zrUtil.merge(markers, newMarkers);
+ }
+ else {
+ html = seriesTooltip;
}
+ dataParams.html = html;
+ });
+
+ switch (singleTooltipModel.get('order')) {
+ case 'valueAsc':
+ singleParamsList.sort(function (a, b) {
+ return a.data - b.data;
+ });
+ break;
+
+ case 'valueDesc':
+ singleParamsList.sort(function (a, b) {
+ return b.data - a.data;
+ });
+ break;
+
+ case 'legendDesc':
+ singleParamsList.reverse();
+ break;
+
+ case 'legendAsc':
+ default:
+ break;
+ }
+
+ singleParamsList.forEach(element => {
Review comment:
Use `zrUtill.each` instead.
There is no polyfill of `forEach` now.
##########
File path: src/util/format.ts
##########
@@ -66,6 +66,13 @@ export function encodeHTML(source: string): string {
});
}
+export function concatTooltipHtml(html: string, value: unknown, dontEncodeHtml = false): string {
+ return (dontEncodeHtml ? html : encodeHTML(html))
+ + (value ? '<strong style="float:right;margin-left:20px;color:#000;">' : '')
Review comment:
In my opinion, we should not replay on the default style of user agent.
`<strong>` may be defined as other style in different environments, that might makes the final effect out of control.
We should better write inline style directly, and use tag like `span` that has lower chance to be style-redefined
##########
File path: test/new-tooltip.html
##########
@@ -0,0 +1,2950 @@
+<!--
+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.
+-->
+
+<!DOCTYPE html>
+
+<head>
+ <meta charset="utf-8">
+ <script src="lib/esl.js"></script>
+ <script src="lib/config.js"></script>
+</head>
+
+<body>
+ <h1>Tooltip in Line Chart</h1>
+ <div id="main1" style="width: 100vw;height:40vh;margin: 0 auto;"></div>
Review comment:
The `<body>` has a `margin: 8px`, here `width: 100vw` make of div overflow the viewport.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -528,8 +533,27 @@ class TooltipView extends ComponentView {
zrUtil.each(item.seriesDataIndices, function (idxItem) {
const series = ecModel.getSeriesByIndex(idxItem.seriesIndex);
+ const data = series.getData();
const dataIndex = idxItem.dataIndexInside;
+ const dims = zrUtil.map(series.coordinateSystem.dimensions, function (coordDim) {
+ return data.mapDimension(coordDim);
+ });
const dataParams = series && series.getDataParams(dataIndex) as TooltipDataParams;
+ let isStacked = false;
+ const stackResultDim = data.getCalculationInfo('stackResultDimension');
+ if (isDimensionStacked(data, dims[0])) {
+ isStacked = true;
+ dims[0] = stackResultDim;
+ }
+ if (isDimensionStacked(data, dims[1])) {
+ isStacked = true;
+ dims[1] = stackResultDim;
+ }
+ dataParams.position = findPointFromSeries({
Review comment:
`dataParams` will be exposed to users in callback.
So every properties on `dataParams` should have a clear meaning for users, and any change in future will be a break change.
I think it's not easy to tell users what this `position`. And it might be changed in future.
So it should better not be mounted on dataParams.
##########
File path: src/model/Series.ts
##########
@@ -526,7 +526,7 @@ class SeriesModel<Opt extends SeriesOption = SeriesOption> extends ComponentMode
: tooltipDimLen
? formatSingleValue(retrieveRawValue(data, dataIndex, tooltipDims[0]))
: formatSingleValue(isValueArr ? value[0] : value);
- const content = formattedValue.content;
+ const content = `<strong style="float:right;margin-left:20px;color:#000;">${formattedValue.content}</strong>`;
Review comment:
In my opinion, we should not replay on the default style of user agent.
`<strong>` may be defined as other style in different environments, that might makes the final effect out of control.
We should better write inline style directly, and use tag like `span` that has lower chance to be style-redefined.
${formattedValue.content} may be contain some html "block" tags (for example in candlestick and boxplot)? not good to encapsulated by a tag inline `<strong>` ?
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -541,23 +565,47 @@ class TooltipView extends ComponentView {
renderMode
});
- if (dataParams) {
- singleParamsList.push(dataParams);
- const seriesTooltip = series.formatTooltip(
- dataIndex, true, null, renderMode as TooltipRenderMode
- );
-
- let html;
- if (zrUtil.isObject(seriesTooltip)) {
- html = seriesTooltip.html;
- const newMarkers = seriesTooltip.markers;
- zrUtil.merge(markers, newMarkers);
- }
- else {
- html = seriesTooltip;
- }
- seriesDefaultHTML.push(html);
+ singleParamsList.push(dataParams);
+ const seriesTooltip = series.formatTooltip(
+ dataIndex, true, null, renderMode as TooltipRenderMode
+ );
+
+ let html;
+ if (zrUtil.isObject(seriesTooltip)) {
+ html = seriesTooltip.html;
+ const newMarkers = seriesTooltip.markers;
+ zrUtil.merge(markers, newMarkers);
+ }
+ else {
+ html = seriesTooltip;
}
+ dataParams.html = html;
+ });
+
+ switch (singleTooltipModel.get('order')) {
+ case 'valueAsc':
+ singleParamsList.sort(function (a, b) {
+ return a.data - b.data;
Review comment:
`data` might be an array, can not be minus directly.
I suggest change the ts type of `CallbackDataParams['data']` and `CallbackDataParams['value']` from `any` to `unknown`. `any` might lead us to use it but not check what it is.
And both `data` and `value` property here is from "raw data". Values in "raw data" is not parsed yet.
For example, if user set
```js
option = {
xAxis: { type: 'time' },
yAxis: {},
series: [{
type: 'line',
data: [
['2012-2-12', 9988],
['2012-2-13', 10021],
]
}]
};
```
The prop `data` would be `['2012-2-12', 9988]` here and prop `value` would be string `'2012-2-12'`.
That date string should not be used to sort.
If really need to sort, we should better user the parsed data, which can be retrieved from like
```ts
const data = series.getData()
const axisDimension = ...
const dataDimension = data.mapDimension(axisDimension);
const val = data.get(dataDimension);
// sort by that val
```
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -752,12 +805,50 @@ class TooltipView extends ComponentView {
html = formatter(params, asyncTicket, callback);
}
- tooltipContent.setContent(html, markers, tooltipModel);
- tooltipContent.show(tooltipModel);
-
this._updatePosition(
tooltipModel, positionExpr, x, y, tooltipContent, params, el
);
+ tooltipContent.setContent(html, markers, tooltipModel, nearPoint.color, positionExpr);
+ tooltipContent.show(tooltipModel, nearPoint.color);
+
+ }
+
+ _getNearestPoint(
+ point: number[],
+ tooltipDataParams: TooltipDataParams | TooltipDataParams[],
+ coord?: CoordinateSystem
+ ): {
+ x: number;
+ y: number;
+ color: ZRColor;
+ } {
+ let dim = '';
+ if (coord && coord.type === 'cartesian2d') {
+ dim = coord.getBaseAxis().dim;
+ }
+ if (!zrUtil.isArray(tooltipDataParams)) {
+ if (!tooltipDataParams.position) {
+ return {
+ x: point[0],
+ y: point[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+ return {
+ x: point[0],
+ y: tooltipDataParams.position[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+
+ const posIndex = +(dim === 'x');
+ const distanceArr = tooltipDataParams.map(params => Math.abs(params.position[posIndex] - point[posIndex]));
+ const index = distanceArr.indexOf(Math.min(...distanceArr));
+ return {
+ x: tooltipDataParams[index]?.position[0] || point[0],
Review comment:
There might be no that `position` prop. `_showTooltipContent` is called in two places. See line 746.
And see the comment above, position info should better not put in this `tooltipDataParams`.
##########
File path: src/model/Series.ts
##########
@@ -545,22 +545,19 @@ class SeriesModel<Opt extends SeriesOption = SeriesOption> extends ComponentMode
seriesName = '';
}
seriesName = seriesName
- ? encodeHTML(seriesName) + (!multipleSeries ? newLine : ': ')
+ ? encodeHTML(seriesName) + (!multipleSeries ? newLine : ' ')
: '';
colorStr = typeof colorEl === 'string' ? colorEl : colorEl.content;
const html = !multipleSeries
- ? seriesName + colorStr
+ ? seriesName + (seriesName ? '<br/>' : '') + '<p style="margin: 8px 0 0;">' + colorStr
Review comment:
in my opinion, do not use `<p>`, in case that p is defined with other css properties like `line-height`, padding, and so on.
It's OK to use div.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -752,12 +805,50 @@ class TooltipView extends ComponentView {
html = formatter(params, asyncTicket, callback);
}
- tooltipContent.setContent(html, markers, tooltipModel);
- tooltipContent.show(tooltipModel);
-
this._updatePosition(
tooltipModel, positionExpr, x, y, tooltipContent, params, el
);
+ tooltipContent.setContent(html, markers, tooltipModel, nearPoint.color, positionExpr);
+ tooltipContent.show(tooltipModel, nearPoint.color);
+
+ }
+
+ _getNearestPoint(
+ point: number[],
+ tooltipDataParams: TooltipDataParams | TooltipDataParams[],
+ coord?: CoordinateSystem
+ ): {
+ x: number;
+ y: number;
+ color: ZRColor;
+ } {
+ let dim = '';
+ if (coord && coord.type === 'cartesian2d') {
+ dim = coord.getBaseAxis().dim;
+ }
+ if (!zrUtil.isArray(tooltipDataParams)) {
+ if (!tooltipDataParams.position) {
+ return {
+ x: point[0],
+ y: point[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+ return {
+ x: point[0],
+ y: tooltipDataParams.position[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+
+ const posIndex = +(dim === 'x');
+ const distanceArr = tooltipDataParams.map(params => Math.abs(params.position[posIndex] - point[posIndex]));
+ const index = distanceArr.indexOf(Math.min(...distanceArr));
+ return {
+ x: tooltipDataParams[index]?.position[0] || point[0],
+ y: tooltipDataParams[index]?.position[1] || point[1],
+ color: tooltipDataParams[index]?.color || tooltipDataParams[index]?.borderColor
Review comment:
Should better have a default color ?
##########
File path: src/util/types.ts
##########
@@ -1034,6 +1036,7 @@ export interface CommonTooltipOption<FormatterParams> {
borderColor?: ColorString
borderRadius?: number
borderWidth?: number
+ boxShadow?: ColorString
Review comment:
As commented above, should not be named as `boxShadow` (and the ts type of css boxShadow is not `ColorString`).
Should better be:
shadowBlur
shadowColor
shadowOffsetX
shadowOffsetY
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -752,12 +805,50 @@ class TooltipView extends ComponentView {
html = formatter(params, asyncTicket, callback);
}
- tooltipContent.setContent(html, markers, tooltipModel);
- tooltipContent.show(tooltipModel);
-
this._updatePosition(
tooltipModel, positionExpr, x, y, tooltipContent, params, el
);
+ tooltipContent.setContent(html, markers, tooltipModel, nearPoint.color, positionExpr);
+ tooltipContent.show(tooltipModel, nearPoint.color);
+
+ }
+
+ _getNearestPoint(
+ point: number[],
+ tooltipDataParams: TooltipDataParams | TooltipDataParams[],
+ coord?: CoordinateSystem
+ ): {
+ x: number;
+ y: number;
+ color: ZRColor;
+ } {
+ let dim = '';
+ if (coord && coord.type === 'cartesian2d') {
+ dim = coord.getBaseAxis().dim;
+ }
+ if (!zrUtil.isArray(tooltipDataParams)) {
+ if (!tooltipDataParams.position) {
+ return {
+ x: point[0],
+ y: point[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+ return {
+ x: point[0],
+ y: tooltipDataParams.position[1],
Review comment:
Do not understand this logic. Why x y are traded differently.
But in fact, the return {x, y} from `_getNearestPoint` are never used?
So probably {x, y} related logic here can be removed?
And this method should only be in charge of getting color ?
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -752,12 +805,50 @@ class TooltipView extends ComponentView {
html = formatter(params, asyncTicket, callback);
}
- tooltipContent.setContent(html, markers, tooltipModel);
- tooltipContent.show(tooltipModel);
-
this._updatePosition(
tooltipModel, positionExpr, x, y, tooltipContent, params, el
);
+ tooltipContent.setContent(html, markers, tooltipModel, nearPoint.color, positionExpr);
+ tooltipContent.show(tooltipModel, nearPoint.color);
+
+ }
+
+ _getNearestPoint(
+ point: number[],
+ tooltipDataParams: TooltipDataParams | TooltipDataParams[],
+ coord?: CoordinateSystem
+ ): {
+ x: number;
+ y: number;
+ color: ZRColor;
+ } {
+ let dim = '';
+ if (coord && coord.type === 'cartesian2d') {
+ dim = coord.getBaseAxis().dim;
+ }
+ if (!zrUtil.isArray(tooltipDataParams)) {
+ if (!tooltipDataParams.position) {
+ return {
+ x: point[0],
+ y: point[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+ return {
+ x: point[0],
+ y: tooltipDataParams.position[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+
+ const posIndex = +(dim === 'x');
Review comment:
Not a good way to determine axis dimension. only suitable for cartesian.
We could use Euclidean distance to avoid that axis dimension determine.
##########
File path: src/util/types.ts
##########
@@ -113,6 +113,8 @@ export interface ECElement extends Element {
hoverState?: 0 | 1 | 2;
selected?: boolean;
+ style?: Dictionary<any>;
Review comment:
Use `unknown` instead of `any`. Avoid `any` spreads.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -541,23 +565,47 @@ class TooltipView extends ComponentView {
renderMode
});
- if (dataParams) {
- singleParamsList.push(dataParams);
- const seriesTooltip = series.formatTooltip(
- dataIndex, true, null, renderMode as TooltipRenderMode
- );
-
- let html;
- if (zrUtil.isObject(seriesTooltip)) {
- html = seriesTooltip.html;
- const newMarkers = seriesTooltip.markers;
- zrUtil.merge(markers, newMarkers);
- }
- else {
- html = seriesTooltip;
- }
- seriesDefaultHTML.push(html);
+ singleParamsList.push(dataParams);
+ const seriesTooltip = series.formatTooltip(
+ dataIndex, true, null, renderMode as TooltipRenderMode
+ );
+
+ let html;
+ if (zrUtil.isObject(seriesTooltip)) {
+ html = seriesTooltip.html;
+ const newMarkers = seriesTooltip.markers;
+ zrUtil.merge(markers, newMarkers);
+ }
+ else {
+ html = seriesTooltip;
}
+ dataParams.html = html;
Review comment:
dataParams will be exposed to users in some callback.
Is it intentionally expose `html` to users in callback?
I think the html is possible to be changed in one day if some new requirements come,
so it should better not to be exposed to users unless really necessary ?
----------------------------------------------------------------
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 #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661647527
@susiwen8 It's beautiful!
----------------------------------------------------------------
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] susiwen8 commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r458143633
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -38,6 +39,55 @@ const vendors = ['', '-webkit-', '-moz-', '-o-'];
const gCssText = 'position:absolute;display:block;border-style:solid;white-space:nowrap;z-index:9999999;';
+function mirrowPos(pos: string): string {
+ pos = pos === 'left'
+ ? 'right'
+ : pos === 'right'
+ ? 'left'
+ : pos === 'top'
+ ? 'bottom'
+ : 'top';
+ return pos;
+}
+
+function assembleArrow(
+ backgroundColor: ColorString,
+ borderColor: ZRColor,
+ arrowPosition: TooltipOption['position']
+) {
+ if (zrUtil.isObject(borderColor) && borderColor.type !== 'pattern') {
Review comment:
TS will throw error that said `Property 'colorStops' does not exist on type 'LinearGradientObject | RadialGradientObject | PatternObject'.
Property 'colorStops' does not exist on type 'PatternObject'`
----------------------------------------------------------------
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] susiwen8 commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r458481565
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -752,12 +805,50 @@ class TooltipView extends ComponentView {
html = formatter(params, asyncTicket, callback);
}
- tooltipContent.setContent(html, markers, tooltipModel);
- tooltipContent.show(tooltipModel);
-
this._updatePosition(
tooltipModel, positionExpr, x, y, tooltipContent, params, el
);
+ tooltipContent.setContent(html, markers, tooltipModel, nearPoint.color, positionExpr);
+ tooltipContent.show(tooltipModel, nearPoint.color);
+
+ }
+
+ _getNearestPoint(
+ point: number[],
+ tooltipDataParams: TooltipDataParams | TooltipDataParams[],
+ coord?: CoordinateSystem
+ ): {
+ x: number;
+ y: number;
+ color: ZRColor;
+ } {
+ let dim = '';
+ if (coord && coord.type === 'cartesian2d') {
+ dim = coord.getBaseAxis().dim;
+ }
+ if (!zrUtil.isArray(tooltipDataParams)) {
+ if (!tooltipDataParams.position) {
+ return {
+ x: point[0],
+ y: point[1],
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+ return {
+ x: point[0],
+ y: tooltipDataParams.position[1],
Review comment:
At first `_getNearestPoint` was intended to find the closet point to cursor (only on cartesian), so I can put `arrow` in right position, but then I found out it's little tricky (at that time, I wasn't aware `tooltip.position` which is simpler), so I just fix `tooltip` `y` coordinate to the `y` coordinate of cursor (soon, I realized it should be `value axis` ), in that case, tooltip was situated `left` or `right` of point.
----------------------------------------------------------------
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] susiwen8 commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r458147432
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -79,6 +129,7 @@ function assembleCssText(tooltipModel: Model<TooltipOption>) {
const transitionDuration = tooltipModel.get('transitionDuration');
const backgroundColor = tooltipModel.get('backgroundColor');
+ const boxShadow = tooltipModel.get('boxShadow');
Review comment:
Tooltip doesn't have those options
----------------------------------------------------------------
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] 100pah commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
100pah commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r458078752
##########
File path: src/util/format.ts
##########
@@ -66,6 +66,13 @@ export function encodeHTML(source: string): string {
});
}
+export function concatTooltipHtml(html: string, value: unknown, dontEncodeHtml = false): string {
Review comment:
In my opinion, do not use language feature of "default parameters", especially for a "public" util method.
echarts code trade those three things the same:
`null`, `undefined` and "no that parameter / no that property".
I think it's not a good practice to distinguish them and not necessary.
But the language feature of "default parameters" only works for undefined and no parameter, but not for null.
----------------------------------------------------------------
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] 100pah commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
100pah commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661650537
@susiwen8 Do you mind upgrade about these below:
When performing regression testing on the existing test cases (`test/*tooltip*`) and new test case (`test/new-tooltip.html`), I found some defects:
## Tooltip position issue
The position is not appropriate in some case:
`test/tooltip-appendToBody.html`
The tooltip is far out of the current chart instance, too high. (That is not about the css 3d)
`test/new-tooltip.html`
## Arrow position issue
`test/new-tooltip.html`
Use the first demo as an exmaple:
Set the `grid.bottom: 0`, and set `tooltip.confine: true`, which confine the tooltip not overflow the DOM of the current chart instance. The arrow becoming wrong.
Aslo, set `grid.right: 10`, and set `tooltip.confine: true`, the arrow is also incorrect.
`tooltip.confine` is useful when there are some `overflow:hidden` setting on the any of the ancestors of the main DOM. So I think it should better to add this test case to `test/new-tooltip.html`.
Is indeed an issue that the location of the tooltip body might makes the "center-vertical" arrow impossible to point to the target point. In this case, I think there probably these solutions:
(A) do not show arrow. (recommended)
Simple but not consistent (but I think that result can be accepted).
(B) make a "oblique" arrow.
Not easy to implement, especially consider the different customized style of the tooltip.
I think not need to bring that complex.
(C) make a "oblique line" instead of arrow.
Easier than (B) but still not consistent.
What's your opinion @Wdingding @susiwen8 @pissang @Ovilia ?
## Do not show tooltip when return `null`/`undefined`
Previously, then `tooltip.formatter` return `null`/`undefined`, tooltip will not be displayed any more.
But currently, it is displayed as:
I think we should better not do that break change.
## Content layout issue
In these cases in `test/new-tooltip.html`:
The text is not at the `vertical middle` of the tooltip, which is not good.
In this case below, the gap is too small. (The upper texts and the lower texts are from different series, should have a larger gap than the title and data in a single series)
Because that is a "default style" of tooltip, it could be used in most of the product scenario. So it is checked carefully.
( @Wdingding what's your opinion about that? )
## About the setting to show arrow
In the current commit:
> When trigger was item and position is top | bottom | left | right, Tooltip will show arrow and it points to current series
But I doubt that in most cases, users will not set `tooltip.position` as top | bottom | left | right .
Thus in most cases, arrow will not be displayed.
@Wdingding @pissang is it OK for that?
----------------------------------------------------------------
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] susiwen8 commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661637238
`legendDesc`
`legendAsc`
`valueDesc`
`valueAsc`
----------------------------------------------------------------
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] susiwen8 commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661621014
> Excellent job!
> A small advice on the naming of `order`: may consider `'valueDesc'`, `'valueAsc'`, `'legendDesc'`, `'legendAsc'`.
Right away
----------------------------------------------------------------
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 #12947: New tooltip design
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-664949568
Also I found the display is wrong in `renderMode: 'rich'`
----------------------------------------------------------------
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] susiwen8 commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661652463
@100pah Sure, I'll fix those soon
----------------------------------------------------------------
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 #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661608153
Wonderful work! The screenshots look nice!
----------------------------------------------------------------
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] susiwen8 commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661654745
> @susiwen8 It's beautiful!
>
> Is it default to have a red border? I'm not sure about the design. Perhaps @Wdingding can take a loot?
No, red border color on screenShot that's because the red series was the closest one to cursor in those case
----------------------------------------------------------------
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] susiwen8 commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-662246281
Arrow position might need lots of improvement, and `arrow` At very beginning, I was trying find `nearest point` to `cursor`, so goes `_getNearestPoint` function, but right now this function cover minority scenario(cartesian) I suggest to put on hold to this feature @pissang @100pah @Ovilia @Wdingding
----------------------------------------------------------------
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 #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-656195254
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
[GitHub] [incubator-echarts] susiwen8 edited a comment on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 edited a comment on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661654745
> @susiwen8 It's beautiful!
>
> Is it default to have a red border? I'm not sure about the design. Perhaps @Wdingding can take a loot?
No, red border color on screenShot that's because the red series was the closest one to cursor in those case @pissang
----------------------------------------------------------------
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 edited a comment on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661608153
Wonderful work! The screenshots look nice! We should make it merged before alpha release
----------------------------------------------------------------
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] susiwen8 commented on pull request #12947: [5.0] New tooltip design
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-665561977
----------------------------------------------------------------
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 #12947: New tooltip design
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r461431291
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -752,12 +810,46 @@ class TooltipView extends ComponentView {
html = formatter(params, asyncTicket, callback);
}
- tooltipContent.setContent(html, markers, tooltipModel);
- tooltipContent.show(tooltipModel);
-
+ tooltipContent.setContent(html, markers, tooltipModel, nearPoint.color, positionExpr);
+ tooltipContent.show(tooltipModel, nearPoint.color);
this._updatePosition(
tooltipModel, positionExpr, x, y, tooltipContent, params, el
);
+
+ }
+
+ _getNearestPoint(
+ point: number[],
+ tooltipDataParams: TooltipDataParams | TooltipDataParams[]
+ ): {
+ color: ZRColor;
+ } {
+ if (!zrUtil.isArray(tooltipDataParams)) {
+ if (!tooltipDataParams.position) {
+ return {
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+ return {
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+
+ const distanceArr = tooltipDataParams.map(params => {
+ let dim = '';
Review comment:
Use `zrUtil.map` instead of native map
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -985,7 +1077,8 @@ function calcTooltipPosition(
): [number, number] {
const domWidth = contentSize[0];
const domHeight = contentSize[1];
- const gap = 5;
+ const gap = 10;
+ const offset = 5;
let x = 0;
Review comment:
I can't get the difference between offset and gap
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -752,12 +810,46 @@ class TooltipView extends ComponentView {
html = formatter(params, asyncTicket, callback);
}
- tooltipContent.setContent(html, markers, tooltipModel);
- tooltipContent.show(tooltipModel);
-
+ tooltipContent.setContent(html, markers, tooltipModel, nearPoint.color, positionExpr);
+ tooltipContent.show(tooltipModel, nearPoint.color);
this._updatePosition(
tooltipModel, positionExpr, x, y, tooltipContent, params, el
);
+
+ }
+
+ _getNearestPoint(
+ point: number[],
+ tooltipDataParams: TooltipDataParams | TooltipDataParams[]
+ ): {
+ color: ZRColor;
+ } {
+ if (!zrUtil.isArray(tooltipDataParams)) {
+ if (!tooltipDataParams.position) {
+ return {
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+ return {
+ color: tooltipDataParams.color || tooltipDataParams.borderColor
+ };
+ }
+
+ const distanceArr = tooltipDataParams.map(params => {
+ let dim = '';
+ if (params.coordinateSystem && params.coordinateSystem.type === 'cartesian2d') {
+ dim = params.coordinateSystem.getBaseAxis().dim;
+ }
+ const posIndex = +(dim === 'x');
+ const distance = Math.abs(params.position[posIndex] - point[posIndex]);
+ delete params.position;
+ delete params.coordinateSystem;
+ return distance;
+ });
+ const index = distanceArr.indexOf(Math.min(...distanceArr));
+ return {
Review comment:
Use `zrUtil.indexOf` instead of native indexOf.
##########
File path: src/component/marker/markerHelper.ts
##########
@@ -81,9 +81,9 @@ function markerTypeCalculatorWithExtent(
? data.getCalculationInfo('stackResultDimension')
: targetDataDim;
- const value = numCalculate(data, calcDataDim, markerType);
+ const value = numCalculate(data, targetDataDim, markerType);
Review comment:
It's better to put it in a separate PR and have further discussion about this change.
----------------------------------------------------------------
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] susiwen8 commented on pull request #12947: New tooltip design
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-664343036
@pissang @100pah Hi, Could you review this? I just upgraded new design to rich mode.
----------------------------------------------------------------
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 edited a comment on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661647527
@susiwen8 It's beautiful!
Is it default to have a red border? I'm not sure about the design. Perhaps @Wdingding can take a loot?
----------------------------------------------------------------
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] susiwen8 commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r458478984
##########
File path: src/util/format.ts
##########
@@ -66,6 +66,13 @@ export function encodeHTML(source: string): string {
});
}
+export function concatTooltipHtml(html: string, value: unknown, dontEncodeHtml = false): string {
Review comment:
Thanks for this, I actually don't know "default parameters" doesn't work for `null` :sweat_smile:
----------------------------------------------------------------
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] Ovilia commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
Ovilia commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661612213
Excellent job!
A small advice on the naming of `order`: may consider `'valueDesc'`, `'valueAsc'`, `'legendDesc'`, `'legendAsc'`.
----------------------------------------------------------------
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 #12947: [5.0] New tooltip design
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-666044709
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] susiwen8 commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r458136357
##########
File path: src/component/marker/MarkerModel.ts
##########
@@ -206,19 +203,16 @@ abstract class MarkerModel<Opts extends MarkerOption = MarkerOption> extends Com
const value = this.getRawValue(dataIndex);
const formattedValue = zrUtil.isArray(value)
? zrUtil.map(value, addCommas).join(', ') : addCommas(value as number);
- const name = data.getName(dataIndex);
+ const name = encodeHTML(data.getName(dataIndex));
Review comment:
I have added `dontEncodeHtml` to `true`, so `name` would be encoded one time.
----------------------------------------------------------------
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 #12947: [5.0] New tooltip design
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947
----------------------------------------------------------------
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 edited a comment on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661647527
@susiwen8 It's beautiful!
Is it default to have a red border? I'm not sure about the design
----------------------------------------------------------------
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 edited a comment on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661608153
Wonderful work! The screenshots look nice! We should make it merged before alpha release
Also, do you mind describing the features like `order` in more detail with screenshots? It will make it easier for us to organize the features in the release article
----------------------------------------------------------------
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] 100pah commented on pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
100pah commented on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-661727624
@Wdingding reminded that:
These cases should not be broken:
`test/candlestick.html`
where each dataItem have more than two items to be displayed.
This kind of cases happen in `candlestick` and `boxplot` series.
----------------------------------------------------------------
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 edited a comment on pull request #12947: New tooltip design
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#issuecomment-664949568
Also I found the display is wrong in `renderMode: 'rich'`
And in the candlestick chart. The newline seems broken.
Perhaps it's caused by this change https://github.com/apache/incubator-echarts/pull/12947/files#diff-a99622711c93ad25fc31052794ad226fL142
----------------------------------------------------------------
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] susiwen8 commented on a change in pull request #12947: Tooltip
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #12947:
URL: https://github.com/apache/incubator-echarts/pull/12947#discussion_r458137911
##########
File path: src/component/marker/markerHelper.ts
##########
@@ -81,9 +81,9 @@ function markerTypeCalculatorWithExtent(
? data.getCalculationInfo('stackResultDimension')
: targetDataDim;
- const value = numCalculate(data, calcDataDim, markerType);
+ const value = numCalculate(data, targetDataDim, markerType);
Review comment:
This was trying to fix #11611, and original PR was #12003
----------------------------------------------------------------
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