echarts icon indicating copy to clipboard operation
echarts copied to clipboard

fix: add width, height, x,y to CustomSeriesRenderItemParamsCoordSys type

Open maddhruv opened this issue 1 year ago • 8 comments

Brief Information

Adding more params -- width, height, x, y to CustomSeriesRenderItemParamsCoordSys type, which can be useful when creating custom rectangle shapes with echarts.

const rectShape = echarts.graphic.clipRectByRect(
    {
        x: start[0],
        y: start[1] - height / 2,
        width: end[0] - start[0],
        height: height,
    },
    {
        x: params.coordSys.x ?? 0,
        y: params.coordSys.y ?? 0,
        width: params.coordSys.width ?? 0,
        height: params.coordSys.height ?? 0,
    }
);

This pull request is in the type of:

  • [ ] bug fixing
  • [ ] new feature
  • [x] others

What does this PR do?

Adding more params to CustomSeriesRenderItemParamsCoordSys type, which can be useful when creating custom rectangle shapes with echarts.

Fixed issues

Details

Before: What was the problem?

TypeScript check fails while creating custom rectangles using coorsSys.

After: How does it behave after the fixing?

Fixing the type interface

Document Info

One of the following should be checked.

  • [x] This PR doesn't relate to document changes
  • [ ] The document should be updated later
  • [ ] The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

  • [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

  • [ ] Please squash the commits into a single one when merging.

Other information

maddhruv avatar Sep 08 '22 19:09 maddhruv

Thanks for your contribution! The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

⚠️ MISSING DOCUMENT INFO: Please make sure one of the document options are checked in this PR's description. Search "Document Info" in the description of this PR. This should be done either by the author or the reviewers of the PR.

echarts-bot[bot] avatar Sep 08 '22 19:09 echarts-bot[bot]

You are probably using bmap

I don't think I am

I am using this chart example - https://echarts.apache.org/examples/en/editor.html?c=custom-profile check Line 44

maddhruv avatar Sep 09 '22 09:09 maddhruv

CustomSeriesRenderItemParamsCoordSys does not contain those properties. You are probably using bmap so the type should be changed with bmap. But this file uses @ts-nocheck, so changing this typing won't help. So I would like to suggest converting types in your own code and leave this PR closed. Thanks for your contribution!

I'm confused as well. The example (https://echarts.apache.org/examples/en/editor.html?c=custom-profile) is working, but the type of params.coordSys seems to be wrong.

@Ovilia Could you give us an example of how to change the types? Or could it be, that the example is outdated?

Thanks!

jjansen72 avatar Sep 13 '22 09:09 jjansen72

The type of params.coordSys is not wrong. Custom series can be used in different coordinates, width, height, x, y exist only when using it with cartesian coordinate system.

Ovilia avatar Sep 14 '22 06:09 Ovilia

exist only when using it with cartesian coordinate system.

That is why I have introduced them as optional properties, as they might be undefined in other cases.

maddhruv avatar Sep 14 '22 09:09 maddhruv

@maddhruv x, y, width, height is only for the cartesian coordinate system. Just as it's not reasonable to reveal child property in parent class, they should be included in CustomSeriesRenderItemParamsCoordSys. I think we could add [key: string]: unknown; to fix the typing problem here.

Ovilia avatar Sep 19 '22 04:09 Ovilia

Yeah @Ovilia I can totally do this

interface CustomSeriesRenderItemParamsCoordSys {
    type: string;
    [key: string]: unknown;
}

But I would still have a problem with echarts.graphic.clipRectByRect where it accepts the RectLike to be of number so I would either have to typecast my values there, to make this work -

const rect = echarts.graphic.clipRectByRect(
    {x: 1},
    {x: (params.coordSys.x ?? 0) as number}
)

maddhruv avatar Sep 19 '22 08:09 maddhruv

@maddhruv Yes, this would be inevitable.

Ovilia avatar Sep 20 '22 06:09 Ovilia

Why not just extend the type with the required properties (x,y,width,height)? Exported from ECharts.

Quest: Create the custom example in TypeScript too. And provide the necessary types. No need to create custom types for things coming from this library. Also avoid any / unknown type or unknown additional property like { [key: string]: unknown }. TS !== TS if not used correctly. In this case the x,y,width,height properties are provided from this libary. Why is there no valid type?

infacto avatar Feb 06 '23 14:02 infacto