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

[Update] wait for Desktop and Mobile svg #227

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

GabeDiniz
Copy link
Contributor

✨ Issue: #226
πŸ”€ Branch: fix/226/loading-animation-on-mobile

✏ What was changed?

  • LoadingAnimation waits for Desktop and Mobile SVG to load before displaying the page

@GabeDiniz GabeDiniz added the bug Something isn't working label Apr 1, 2024
@GabeDiniz GabeDiniz self-assigned this Apr 1, 2024
@GabeDiniz GabeDiniz requested a review from aidantrabs as a code owner April 1, 2024 19:34
@GabeDiniz GabeDiniz linked an issue Apr 1, 2024 that may be closed by this pull request
@teoh4770
Copy link
Contributor

teoh4770 commented Apr 1, 2024

@GabeDiniz
Copy link
Contributor Author

Is it possible to do this @GabeDiniz https://medium.com/geekculture/detecting-mobile-vs-desktop-browsers-in-javascript-ad46e8d23ce5

Yea its possible! I think I know how to do this with width sizes. The breakpoint for the 2 svgs are: Mobile < 640px <= Desktop
Do you think that would be fine?

@teoh4770
Copy link
Contributor

teoh4770 commented Apr 1, 2024

Is it possible to do this @GabeDiniz medium.com/geekculture/detecting-mobile-vs-desktop-browsers-in-javascript-ad46e8d23ce5

Yea its possible! I think I know how to do this with width sizes. The breakpoint for the 2 svgs are: Mobile < 640px <= Desktop Do you think that would be fine?

yeah people has done that, but it is not that great

@teoh4770
Copy link
Contributor

teoh4770 commented Apr 1, 2024

In the posts, it talks about the best practice of doing something like this.
I prefer this reasoning here "Use libraries to simplify detection and improve accuracy."

Best Practices

  • Favor feature detection over user agent string detection whenever possible, as it is more reliable and future-proof.
  • Use libraries to simplify detection and improve accuracy.
  • Combine multiple techniques for more robust detection.
  • Continuously test and update your detection code to ensure compatibility with new devices and browsers.

@teoh4770
Copy link
Contributor

teoh4770 commented Apr 1, 2024

you can try use library like this? https://www.npmjs.com/package/react-device-detect

@juancwu
Copy link
Contributor

juancwu commented Apr 1, 2024

In the posts, it talks about the best practice of doing something like this. I prefer this reasoning here "Use libraries to simplify detection and improve accuracy."

Best Practices

* Favor feature detection over user agent string detection whenever possible, as it is more reliable and future-proof.

* Use libraries to simplify detection and improve accuracy.

* Combine multiple techniques for more robust detection.

* Continuously test and update your detection code to ensure compatibility with new devices and browsers.

I don't particularly like the idea of using libraries just because it seems better. In our case, since the landing page will likely only be used this year, and next year will be updated, using a library is good. However, the use of a library also adds a new dependency, which can also be tech debt that we don't directly own. For something like detecting if its mobile/desktop, it can be easy enough that we don't have to rely on a library.

At the end of the day, people who make the libraries are human, and inaccuracy may still be present.

@GabeDiniz
Copy link
Contributor Author

@juancwu @teoh4770 updated it :)

@teoh4770
Copy link
Contributor

teoh4770 commented Apr 1, 2024

seems like it works :D @GabeDiniz

@GabeDiniz GabeDiniz merged commit a2fd173 into main Apr 2, 2024
3 checks passed
@aidantrabs aidantrabs deleted the fix/226/loading-animation-on-mobile branch June 3, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fix] LoadingAnimation on Mobile
4 participants