(feat): marker upgrade for stock charts #20166
Conversation
|
Thanks for your contribution! The pull request is marked to be |
|
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-20166@0ac383d |
echarts-bot
bot
added
the
PR: awaiting doc
|
Document changes are required in this PR. Please also make a PR to apache/echarts-doc for document changes and update the issue id in the PR description. When the doc PR is merged, the maintainers will remove the |
| @@ -53,6 +53,7 @@ export interface MarkerPositionOption { | |||
| // Absolute position, px or percent string | |||
| x?: number | string | |||
| y?: number | string | |||
| relativeTo?: 'screen' | 'coordinate' | |||
There was a problem hiding this comment.
- I think that using
screenmight misleads users, because the the chart is just a part of a html webpage, and in CSS thescreenalready has some specific meaning (also doesn't refer to the real screen but the semantic is well-known).
But I haven't come up with a perfect name yet.
Some candidates:
viewport(also has specific meaning in HTML/CSS but better than screen?)global(a more general term?)global-container
- The term
coordinateis a point in a coordinate system, rather than coordinate system itself.
Should this option becoordinateSystemorcoordinate-system, since the termcoordinateSystemhas been used in echarts option?
There was a problem hiding this comment.
global sounds good to me, only that it's an adj. instead of noun. In the case of relativeTo something, it may be better to follow with a noun. GPT suggests container and coordinateSystem and I think it's Okay. What do you think?
| x: this.cx - radiusExtent[1], | ||
| y: this.cy - radiusExtent[1], | ||
| width: radiusExtent[1] * 2, | ||
| height: radiusExtent[1] * 2 |
There was a problem hiding this comment.
Should we support r0 > r1? I tried it just now and it works.
It might be more robustness if take the max(radiusExtent[0], radiusExtent[1]) here.
There was a problem hiding this comment.
It seems the contain (d2 <= r * r && d2 >= r0 * r0) assumes r0 <= r. If max(radiusExtent[0], radiusExtent[1]) is used here, maybe we should also change other places, or just assumes r0 <= r until other requirement.
| @@ -146,6 +146,7 @@ export function dataTransform( | |||
| // x y is provided | |||
| if (item.coord == null || !isArray(dims)) { | |||
| item.coord = []; | |||
| item.value = numCalculate(data, data.mapDimension(dims[1]), item.type); | |||
There was a problem hiding this comment.
Should it be
numCalculate(data, data.mapDimension(seriesModel.getBaseAxis()), item.type);?
There was a problem hiding this comment.
OK. Thanks for correction.
Brief Information
This pull request is in the type of:
What does this PR do?
Enhance the ability of markerPoint so that we can do more for stock charts.
Fixed issues
#19578
Details
Before: What was the problem?
We can only set the position of markPoints according to the whole canvas, but not to the grid area in pixels.
After: How does it behave after the fixing?
With
relativeTo: 'coordinate', we can set the position in pixels according to the grid area.Document Info
New API:
markPoint.data.relativeTo: 'screen' | 'coordinate'One of the following should be checked.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
Other information