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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Jul 15, 2021
Merged

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

merged 14 commits into from
Jul 15, 2021

Conversation

g7i
Copy link
Contributor

@g7i g7i commented Mar 3, 2021 •
edited
Loading

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 •
edited
Loading

@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
plainheart
plainheart previously requested changes May 6, 2021
View reviewed changes
src/component/tooltip/TooltipHTMLContent.ts Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 19, 2021
@g7i g7i requested a review from plainheart May 19, 2021 14:56
plainheart
plainheart reviewed May 19, 2021
View reviewed changes
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 •
edited
Loading

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.

plainheart
plainheart reviewed Jul 6, 2021
View reviewed changes
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
@g7i @plainheart
Update src/component/tooltip/TooltipHTMLContent.ts
607bf95 
Co-authored-by: Zhongxiang.Wang <yhen@all-my-life.cn>
@g7i @plainheart
Update src/component/tooltip/TooltipHTMLContent.ts
34847b4 
Co-authored-by: Zhongxiang.Wang <yhen@all-my-life.cn>
g7i and others added 2 commits July 6, 2021 14:04
@g7i @plainheart
Update src/component/tooltip/TooltipHTMLContent.ts
71d2be3 
Co-authored-by: Zhongxiang.Wang <yhen@all-my-life.cn>
@g7i @plainheart
Update src/component/tooltip/TooltipHTMLContent.ts
37d9ab8 
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
@plainheart
fix(tooltip):
28f7c33 
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`.
@plainheart plainheart mentioned this pull request Aug 24, 2021
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plainheart plainheart plainheart left review comments

@susiwen8 susiwen8 susiwen8 left review comments

@pissang pissang pissang approved these changes

Assignees
No one assigned
Labels
PR: first-time contributor priority: high size/M
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

[bug] tooltip arrow should follow the specified borderWidth
5 participants
@g7i @pissang @plainheart @susiwen8 @Ovilia