-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
Optimize: disable tooltip transition at first show #13067
Conversation
Thanks for your contribution! |
@@ -153,7 +151,7 @@ function assembleCssText(tooltipModel: Model<TooltipOption>) { | |||
cssText.push('box-shadow:' + boxShadow); | |||
// Animation transition. Do not animate when transitionDuration is 0. | |||
// If tooltip show arrow, then disable transition | |||
transitionDuration | |||
!isFirstShow && transitionDuration | |||
&& !(['top', 'left', 'bottom', 'right'].includes(tooltipModel.get('position') as string)) | |||
&& tooltipModel.get('trigger') !== 'item' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't notice it before. But we should use zrUtil.indexOf
instead of includes
here.
Also in the assembleArrow
function also has includes
usage. Which should be changed.
Hi @susiwen8. Just a suggestion that we should use more clear commits message. For example https://www.conventionalcommits.org/en/v1.0.0-beta.4/#summary It's not a good style to use a very general and unclear message like "Coorection base CR" |
Roger that 😛 |
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
Brief Information
This pull request is in the type of:
What does this PR do?
The first time of showing
tooltip
on each chart, because oftransition
,tooltip
rise from the bottom. So disable transition on the first timeFixed issues
Details
Before: What was the problem?
After: How is it fixed in this PR?
Usage
Are there any API changes?
Related test cases or examples to use the new APIs
NA.
Others
Merging options
Other information