-
Notifications
You must be signed in to change notification settings - Fork 19.7k
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
showLoading align center #12414
showLoading align center #12414
Conversation
Thanks for your contribution! The pull request is marked to be |
maskColor: 'rgba(255, 255, 255, 0.8)', | ||
showAnimation: true, | ||
color: '#c23531', |
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.
Perhaps the name is better to be showSpinner
?
showAnimation: true, | ||
color: '#c23531', | ||
arcRadius: 10, | ||
lineWidth: 5, |
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.
spinnerRadius
?
test/runTest/actions/loading.json
Outdated
@@ -0,0 +1 @@ | |||
[{"name":"Action 1","ops":[],"scrollY":0,"scrollX":0,"timestamp":1586513969778},{"name":"Action 2","ops":[],"scrollY":502,"scrollX":0,"timestamp":1586514012325}] |
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.
It seems there is nothing in the action. Please remove it.
src/loading/default.js
Outdated
var cx = api.getWidth() / 2; | ||
var textWidth = textContain.getWidth(opts.text, font); | ||
var r = opts.showSpinner ? arc.shape.r : 0; | ||
// cx = (containerWidth - (arcDiameter + labelRectWidth) - textDistance - textWidth) / 2 |
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.
arc is in the scope of if (opts.showSpinner)
above. It should not been accessed here. Consider it's a let
instead of var
src/loading/default.js
Outdated
var r = opts.showSpinner ? arc.shape.r : 0; | ||
// cx = (containerWidth - (arcDiameter + labelRectWidth) - textDistance - textWidth) / 2 | ||
// textDistance needs to be calculated when both animation and text exist | ||
var cx = (api.getWidth() - r * 4 - (opts.showSpinner && textWidth ? 10 : 0) - textWidth) / 2 |
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.
Why it's r * 4
instead of r * 2
?
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.
4 * r === arcDiameter + labelRectWidth
- the diameter of spinner is 2*r
labelRect.setShape({..., width: r * 2, ...})
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?
Enhance the showLoading(#11790)
Fixed issues
Details
Before: What was the problem?
showLoading align center by the center of circle in animation
After: How is it fixed in this PR?
showLoading align center by the center of animation and text
Usage
Are there any API changes?
Related test cases or examples to use the new APIs
NA.
Others
Merging options
Other information