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

bug(tooltip): tooltip arrow will follow borderWidth. close #14373 #14393

Merged
merged 14 commits into from Jul 15, 2021

Conversation

g7i
Copy link
Contributor

@g7i g7i commented Mar 3, 2021

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Tooltip arrow will follow the specified borderWidth in tooltip.

Fixed issues

fixes: #14373

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

Checkout test/scatter.html

Others

Merging options

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

Other information

@echarts-bot
Copy link

echarts-bot bot commented Mar 3, 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.

@Ovilia Ovilia added this to the 5.1.1 milestone Apr 6, 2021
@pissang pissang modified the milestones: 5.1.1, 5.1.2 Apr 22, 2021
@g7i
Copy link
Contributor Author

g7i commented Apr 26, 2021

Hey, do I need to resolve the conflicts or the maintainer will resolve?

@pissang
Copy link
Contributor

pissang commented Apr 26, 2021

@g7i Hi, It will be great if you can resolve the conflicts

@pissang
Copy link
Contributor

pissang commented Apr 27, 2021

@g7i Please don't commit the files in dist and i18n folder. They are only updated before release

@g7i
Copy link
Contributor Author

g7i commented Apr 27, 2021

@g7i Please don't commit the files in dist and i18n folder. They are only updated before release

Ohh okay, sorry did it by mistake. Now reverted the changes. Thanks....

@pissang pissang requested a review from susiwen8 May 5, 2021 02:28
src/component/tooltip/TooltipHTMLContent.ts Outdated Show resolved Hide resolved
Copy link
Member

@plainheart plainheart left a comment

Choose a reason for hiding this comment

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

Left my several thoughts. We can discuss them together whether they are reasonable or not.

src/component/tooltip/TooltipView.ts Outdated Show resolved Hide resolved
src/component/tooltip/TooltipHTMLContent.ts Outdated Show resolved Hide resolved
src/component/tooltip/TooltipHTMLContent.ts Outdated Show resolved Hide resolved
src/component/tooltip/TooltipView.ts Show resolved Hide resolved
@pissang pissang modified the milestones: 5.1.2, 5.2.0 May 27, 2021
@pissang
Copy link
Contributor

pissang commented May 27, 2021

Seems we can't make it merged in 5.1.2. Let's continue it in the next 5.2.0

@g7i
Copy link
Contributor Author

g7i commented May 30, 2021

Okay, thanks. I was away for a while that's why unable to work on it. Will fix it asap.

@g7i
Copy link
Contributor Author

g7i commented Jun 12, 2021

@pissang @plainheart I have addressed all the comments for changes. Please re-review it. Thank you.

src/component/tooltip/TooltipHTMLContent.ts Outdated Show resolved Hide resolved
src/component/tooltip/TooltipHTMLContent.ts Outdated Show resolved Hide resolved
src/component/tooltip/TooltipHTMLContent.ts Outdated Show resolved Hide resolved
g7i and others added 2 commits July 6, 2021 14:03
Co-authored-by: Zhongxiang.Wang <yhen@all-my-life.cn>
Co-authored-by: Zhongxiang.Wang <yhen@all-my-life.cn>
g7i and others added 2 commits July 6, 2021 14:04
Co-authored-by: Zhongxiang.Wang <yhen@all-my-life.cn>
Co-authored-by: Zhongxiang.Wang <yhen@all-my-life.cn>
@g7i
Copy link
Contributor Author

g7i commented Jul 6, 2021

Thanks for helping me out :)

@plainheart
Copy link
Member

@g7i Really thank you for the work!
@pissang Please help take a double check. I think it should be working well.

@plainheart
Copy link
Member

Is it ready to be merged now?

It looks good to me now. If other maintainers have no issue with this, it will be merged.

@g7i
Copy link
Contributor Author

g7i commented Jul 6, 2021

Is it ready to be merged now?

It looks good to me now. If other maintainers have no issue with this, it will be merged.

Alright, thanks 🎉

@pissang
Copy link
Contributor

pissang commented Jul 15, 2021

LGTM. @g7i Thanks for keeping improving your PR and @plainheart thanks for the detailed review.

@pissang pissang dismissed plainheart’s stale review July 15, 2021 06:32

already approved by him

@pissang pissang merged commit 0d30818 into apache:master Jul 15, 2021
@echarts-bot
Copy link

echarts-bot bot commented Jul 15, 2021

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

plainheart added a commit that referenced this pull request Aug 24, 2021
1) tweak the calculation of tooltip offset to make it look not so near to the target element. #14393
2) remove unnecessary `getOuterSize` method beacuse `getSize` is now using `offsetWidth/offsetHeight` that contains the `borderWidth`.
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.

[bug] tooltip arrow should follow the specified borderWidth
5 participants