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

perf(loading spinner): optimize loading spinner and inline code + css animation #6821

Merged
merged 7 commits into from
Jun 13, 2024

Conversation

VIKTORVAV99
Copy link
Member

Description

Same as #6819 but uses CSS for the animation instead. Looks to be quicker than the animate elements in the code.
Closes: #6819

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@VIKTORVAV99
Copy link
Member Author

@tonypls if you think this works better feel free to merge this instead 🙂

@tonypls
Copy link
Collaborator

tonypls commented Jun 11, 2024

Maybe @Alportan can confirm the animation is perfect? Testing instructions, open dev tools, click on network tab, set throttling to fast 3G, reload tab and watch the spinner spin

@Alportan
Copy link
Contributor

Alportan commented Jun 11, 2024

Maybe @Alportan can confirm the animation is perfect? Testing instructions, open dev tools, click on network tab, set throttling to fast 3G, reload tab and watch the spinner spin

The animation looks good 🙌

Maybe a small small thing, more regarding the logo SVG itself (*puts brand police cap on ⚡️👮‍♂️🚨), the zap in the middle seems a tiny bit bigger than the one we use in the logo asset. By far a nitpick and not a blocker to this PR 😅 🙈

image

☝️ I overlayed my asset on the screenshot of the spinner 🙌

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jun 11, 2024

Maybe @Alportan can confirm the animation is perfect? Testing instructions, open dev tools, click on network tab, set throttling to fast 3G, reload tab and watch the spinner spin

The animation looks good 🙌

Maybe a small small thing, more regarding the logo SVG itself (*puts brand police cap on ⚡️👮‍♂️🚨), the zap in the middle seems a tiny bit bigger than the one we use in the logo asset. By far a nitpick and not a blocker to this PR 😅 🙈

image

☝️ I overlayed my asset on the screenshot of the spinner 🙌

Well that's awkward as I copied the one we use for the favicon and animated that (it had cleaner proportions).
Do you have the original asset as SVG? if so I can update all our icons to use that later. Preferable in 512 or 1024 sizes?

@Alportan
Copy link
Contributor

Well that's awkward as I copied the one we use for the favicon and animated that (it had cleaner proportions). Do you have the original asset as SVG? if so I can update all our icons to use that later. Preferable in 512 or 1024 sizes?

Haha, I don't know about the favicon, but I can definitely provide the SVGs. I've attached them here:

logo_1024
logo_512

web/src/index.css Outdated Show resolved Hide resolved
Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Minor comment, otherwise LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants