-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
refactor Spinner, kill SVGSpinner #2477
Conversation
- remove all wrapper elements so svg is the root - overflow:visible means no need to switch viewBox anymore! - animate only the track head path, not the whole thing - add style prop
.#{$ns}-spinner { | ||
@include spinner-size($spinner-width-factor, $spinner-stroke-width, $spinner-speed); | ||
// allow paths to overflow container -- critical for edges of circles! | ||
overflow: visible; | ||
vertical-align: top; |
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.
is vertical align necessary? is this going to mess with existing uses?
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.
hmm good thought. i'll change to middle
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.
is it even needed?
need to add |
vertical-align, set height alongside widthPreview: documentation | landing | table |
@@ -193,8 +193,6 @@ export const SPINNER = `${NS}-spinner`; | |||
export const SPINNER_HEAD = `${SPINNER}-head`; | |||
export const SPINNER_NO_SPIN = `${NS}-no-spin`; |
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.
while we're at it, thoughts on a better name for this? something more generic like .pt-static
maybe? or maybe indeterminate vs determinate loading?
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.
meh i have no problems with this name. there's also PROGRESS_NO_STRIPES/ANIMATION
<div className={classes}> | ||
<div className={Classes.SPINNER_SVG_CONTAINER}>{content}</div> | ||
</div> | ||
<svg className={classes} viewBox="0 0 100 100"> |
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.
I don't fully understand SVG/viewBox and stuff, but it's amazing that you were able to dramatically simplify
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.
yes!!
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.
design +1, looking great! defer on the implementation
Spinner
markup considerably:overflow:visible
means no need to switch viewBox anymore!SVGSpinner
sinceSpinner
can now be used inside an SVG (no DOM elements)Classes.SPINNER_SVG_CONTAINER
andClasses.SVG_SPINNER