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

fix: #12812 #12834

Merged
merged 2 commits into from
Jun 23, 2020
Merged

fix: #12812 #12834

merged 2 commits into from
Jun 23, 2020

Conversation

liulinboyi
Copy link
Contributor

@liulinboyi liulinboyi commented Jun 19, 2020 •
edited by susiwen8
Loading

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

when alwaysShowContent is true change or rotation window size and restore will hide tooltip

Fixed issues

Close #12812

Details

Before: What was the problem?

1- display echart tooltip exemple: https://echarts.apache.org/examples/en/editor.html?c=line-tooltip-touch

2- add option 'alwaysShowContent: true' for tooltip

3- click on the chart for display tooltip

4- change window size or rotation

the tooltip move, not corresponding to the selected point position

After: How is it fixed in this PR?

when alwaysShowContent is true change or rotation window size and restore will hide tooltip

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

NA.

Others

Merging options

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

Other information

susiwen8
susiwen8 requested changes Jun 19, 2020
View reviewed changes
src/echarts.js Outdated Show resolved Hide resolved
src/echarts.js Outdated Show resolved Hide resolved
@liulinboyi liulinboyi force-pushed the fix-12812 branch 2 times, most recently from cb7348a to 4c91a19 Compare June 19, 2020 15:07
@plainheart
Copy link
Member

Why hide tooltip rather than relocate it to the proper coordinate?
And I think doing it here is a bit inappropriate.

@plainheart plainheart self-assigned this Jun 22, 2020
@liulinboyi
Copy link
Contributor Author

Why hide tooltip rather than relocate it to the proper coordinate?
And I think doing it here is a bit inappropriate.

Let me have a try.

@liulinboyi
Copy link
Contributor Author

Why hide tooltip rather than relocate it to the proper coordinate?
And I think doing it here is a bit inappropriate.

@plainheart The effect is as follows，Please help review, thanks!
move

plainheart
plainheart reviewed Jun 22, 2020
View reviewed changes
src/echarts.js Outdated Show resolved Hide resolved
src/echarts.js Outdated Show resolved Hide resolved
@liulinboyi liulinboyi force-pushed the fix-12812 branch 2 times, most recently from 5e6690d to ad26dc5 Compare June 22, 2020 05:27
@liulinboyi
Copy link
Contributor Author

@plainheart All the problems are solved, Please help review, thanks!

plainheart
plainheart requested changes Jun 22, 2020
View reviewed changes
src/component/tooltip/TooltipContent.js Outdated Show resolved Hide resolved
src/component/tooltip/TooltipContent.js Outdated Show resolved Hide resolved
src/component/tooltip/TooltipContent.js Show resolved Hide resolved
@plainheart
Copy link
Member

plainheart commented Jun 22, 2020 •
edited
Loading

Finally, the same changes should be applied to TooltipRichContent, which will be used when renderMode is richText.

@liulinboyi
Copy link
Contributor Author

Finally, the same changes should be applied to TooltipRichContent, which will be used when renderMode is richText.

@plainheart All the problems are solved, Please help review, thanks!

@liulinboyi
Copy link
Contributor Author

Finally, the same changes should be applied to TooltipRichContent, which will be used when renderMode is richText.

@plainheart Thank you for your guidance.

@plainheart plainheart added this to the 4.9.0 milestone Jun 22, 2020
@plainheart
Copy link
Member

@liulinboyi Thank you, great work! All looks good to me now, please @pissang have a look.

@liulinboyi
Copy link
Contributor Author

Perhaps the _moveTooltip can be renamed to a more specific name like _moveTooltipIfResized

Others all look good to me. Thanks for the wonderful work! @liulinboyi @plainheart

Ok, no problem.

@liulinboyi
Copy link
Contributor Author

Perhaps the _moveTooltip can be renamed to a more specific name like _moveTooltipIfResized

Others all look good to me. Thanks for the wonderful work! @liulinboyi @plainheart

Thank you for your guidance. @pissang I have fix some spell error and the _moveTooltip was renamed, Please help review, thanks!

pissang
pissang previously approved these changes Jun 23, 2020
View reviewed changes
@pissang
Copy link
Contributor

pissang commented Jun 23, 2020

Seems there are still two request changes from @plainheart @susiwen8 need to be resolved before this PR can ben merged

susiwen8
susiwen8 previously approved these changes Jun 23, 2020
View reviewed changes
@liulinboyi
Copy link
Contributor Author

Seems there are still two request changes from @plainheart @susiwen8 need to be resolved before this PR can ben merged

@pissang All request changes are done. Can i fix other files spell error?

plainheart
plainheart requested changes Jun 23, 2020
View reviewed changes
test/tooltip-windowResize.html Outdated Show resolved Hide resolved
@liulinboyi liulinboyi dismissed stale reviews from susiwen8 and pissang via 5c41a15 June 23, 2020 07:04
@plainheart
Copy link
Member

plainheart commented Jun 23, 2020 •
edited
Loading

Seems there are still two request changes from @plainheart @susiwen8 need to be resolved before this PR can ben merged

@pissang All request changes are done. Can i fix other files spell error?

Merged. Thanks for your contribution!
And for the request to fix spelling error, I think you are welcome and free to do it.

@plainheart plainheart merged commit 1f31de3 into apache:master Jun 23, 2020
@echarts-bot
Copy link

echarts-bot bot commented Jun 23, 2020

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

plainheart added a commit to plainheart/echarts that referenced this pull request Oct 2, 2020
@plainheart
chore(tooltip): migrate changes in PR apache#12834 to next branch.
61c07bb 
* See apache#12834
@plainheart plainheart mentioned this pull request Oct 2, 2020
Merged
5 tasks
pissang added a commit that referenced this pull request Oct 9, 2020
@pissang
Merge pull request #13383 from plainheart/feat/tooltip-class
25ceb56 
feat(tooltip): allow adding classes to tooltip DOM and migrate changes to next branch in #12834.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plainheart plainheart plainheart approved these changes

@susiwen8 susiwen8 susiwen8 left review comments

@pissang pissang pissang left review comments

Assignees

@pissang pissang

@plainheart plainheart

Labels
None yet
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.

tooltip position after resize or rotate window
4 participants
@liulinboyi @plainheart @pissang @susiwen8