-
Notifications
You must be signed in to change notification settings - Fork 5
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
[#49] update Loading spinner #73
base: main
Are you sure you want to change the base?
Conversation
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.
This looks good to me so far and the todos make sense 👍
, borderRadius: "50%" | ||
, background: "linear-gradient(to right, " <> color <> " 10%, rgba(255, 255, 255, 0) 42%)" |
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.
Shouldn't we just fade from color
with 100% opacity to color
with 0% opacity? I think this would remove the need for specifying a background color to the loader.
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.
Did you by chance get to try this out @kimispencer ?
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 I am misunderstanding your comment, but the new design has a hard edge, and then fades at the end of the circle... I wasn't able to accomplish this one-directional fade on the border without using a linear-gradient
and thus needed background. Let me know if you meant something else.
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.
Oh, sorry, I think I didn't explain myself properly. I thought of using border gradients for that, but it's just not possible. My main concern with this is that, since the background is a solid color, we won't be able to use loading spinners over text, otherwise this would happen:
Maybe we should use baked images or SVG for this, since we have a closed set of possible colors?
I don't think we'll be using loaders in all colors we offer, perhaps we should just stick to 2 or 3 colors? Would make this easier if we go down the image/SVG path.
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.
Yeah I was trying to use a CSS-only approach but it does result in the spinner not having an opaque/transparent center... I see your point that this might not look good over text. I think rather than a baked in image it would be more flexible (+ look cleaner as vector) using SVG. I'll implement a version using that approach 👍
, borderRadius: "50%" | ||
, background: "linear-gradient(to right, " <> color <> " 10%, rgba(255, 255, 255, 0) 42%)" |
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.
Oh, sorry, I think I didn't explain myself properly. I thought of using border gradients for that, but it's just not possible. My main concern with this is that, since the background is a solid color, we won't be able to use loading spinners over text, otherwise this would happen:
Maybe we should use baked images or SVG for this, since we have a closed set of possible colors?
I don't think we'll be using loaders in all colors we offer, perhaps we should just stick to 2 or 3 colors? Would make this easier if we go down the image/SVG path.
Note, new svg animation approach requires #82 to access SVG module |
#82 has been merged |
@kimispencer is this ready enough to include in the tooltip release? |
@spicydonuts yes, sorry for the delay! Let me update with our SVG stuff and should be ready to go :) |
Resolves #49