Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(types) Export cbs and their parameter types #14871

Merged
merged 1 commit into from
May 18, 2021

Conversation

dougalg
Copy link
Contributor

@dougalg dougalg commented May 6, 2021

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Adds type exports for several callbacks and their parameters

Fixed issues

Details

Before: What was the problem?

It was difficult to implement properly types callback functions

After: How is it fixed in this PR?

It should now be possible to implement properly typed callbacks

Misc

  • The API has been changed (apache/echarts-doc#xxx).
  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

As far as I could tell, there are no tests needed for type exports, but if I missed them, please let me know, and I will try to add them.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

@echarts-bot
Copy link

echarts-bot bot commented May 6, 2021

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.

@@ -0,0 +1,25 @@
import type {
Copy link
Contributor

@pissang pissang May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs Apache License in the header.

IMO these callback types can be exported in the option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I was considering putting them in there, but I initially wasn't sure how many exports I'd be adding and I didn't want to bloat that file too much. But I think this doesn't add too much bloat, although there are some more callbacks I didn't include as I wasn't sure exactly how they should be exported.

@dougalg dougalg force-pushed the fix-14277 branch 2 times, most recently from 6aad0e3 to 9b08a0e Compare May 6, 2021 12:55
@dougalg dougalg marked this pull request as ready for review May 7, 2021 02:29
@dougalg
Copy link
Contributor Author

dougalg commented May 7, 2021

@pissang I have moved this PR to "awaiting review" from WIP, whenever you (or others) are ready to do a final/next review.

Thank you again!

@@ -1235,7 +1235,7 @@ type TooltipBoxLayoutOption = Pick<
/**
* Position relative to the hoverred element. Only available when trigger is item.
*/
interface PositionCallback {
export interface PositionCallback {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PositionCallback can be renamed to TooltipPositionCallback to avoid potential conflicts when exported

LabelLayoutOptionCallbackParams,
LabelLayoutOptionCallback,
PositionCallback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PositionCallback is only used in the tooltip. So it can also be TooltipComponentPositionCallback

@dougalg
Copy link
Contributor Author

dougalg commented May 7, 2021

@pissang I believe I have addressed your comments. I also noticed some inconsistencies in my naming patterns and renamed a couple of the exports to be more consistent. Thank you very much

@pissang pissang modified the milestones: 5.1.2, 5.2.0 May 7, 2021
@pissang pissang merged commit 7c4c93c into apache:master May 18, 2021
@echarts-bot
Copy link

echarts-bot bot commented May 18, 2021

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants