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.

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

kimispencer
Copy link
Contributor

@kimispencer kimispencer commented Jul 8, 2019

Resolves #49

Screen Shot 2019-07-08 at 1 17 04 PM

Screen Shot 2019-07-08 at 1 17 13 PM

@kimispencer kimispencer self-assigned this Jul 8, 2019
@kimispencer kimispencer requested a review from megamaddu July 9, 2019 00:00
Copy link
Member

@megamaddu megamaddu left a 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 👍

@kimispencer kimispencer changed the title [WIP, #49] update Loading spinner [#49] update Loading spinner Jul 16, 2019
, borderRadius: "50%"
, background: "linear-gradient(to right, " <> color <> " 10%, rgba(255, 255, 255, 0) 42%)"
Copy link
Contributor

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.

Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor

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:
Screen Shot 2019-08-06 at 10 13 21

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.

Copy link
Contributor Author

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 👍

src/Lumi/Components/Color.purs Show resolved Hide resolved
src/Lumi/Components/Loader.purs Show resolved Hide resolved
, borderRadius: "50%"
, background: "linear-gradient(to right, " <> color <> " 10%, rgba(255, 255, 255, 0) 42%)"
Copy link
Contributor

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:
Screen Shot 2019-08-06 at 10 13 21

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.

@kimispencer
Copy link
Contributor Author

Note, new svg animation approach requires #82 to access SVG module

@megamaddu
Copy link
Member

#82 has been merged

@megamaddu
Copy link
Member

@kimispencer is this ready enough to include in the tooltip release?

@kimispencer
Copy link
Contributor Author

@spicydonuts yes, sorry for the delay! Let me update with our SVG stuff and should be ready to go :)

@megamaddu megamaddu changed the base branch from master to main June 22, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update loading spinner
3 participants