-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
WebApp manifest added to allow users "Add to Homescreen" documentations #2377
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Deploy preview for reactjs ready! Built with commit 7fe5098 |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
gatsby-config.js
Outdated
background_color: '#20232a', | ||
theme_color: '#20232a', | ||
display: 'standalone', | ||
icon: 'src/images/react_logo.png', |
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 think this is right. The URL needs to be a web accessible address. This is the source location, but you can't load e.g. https://deploy-preview-2377--reactjs.netlify.com/src/images/react_logo.png
So this just makes the "Add to Homescreen" popup spin for a while before it gives up on trying to load the icon.
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.
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.
Hm, I guess I'm just saying that it doesn't seem to work.
iOS+Safari ignores it entirely and shows a thumbnail of the homepage, and Android+Chrome shows the correct icon for reactjs.org (without this PR even being added).
What am I overlooking?
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've added some commits can you check now?
So this repository already had gatsby-plugin-manifest
installed but it was not used so instead of installing another version I used the same plugin.
Anyway now I've added a legacy: true
which is supposed to fix issue for IOS. If that doesn't work then installing latest version of gatsby-plugin-manifest
would be a better choice.
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.
Seems like the behavior is the same as I described before.
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.
So the problem was currently there is gatsby-plugin-manifest@2.0.5
installed which seems to not add apple meta tags in html and since the newer versions of the plugin are dependent on newer versions of other packages. we would have to update a lot of plugins (which may break stuff)
So instead of that I managed to fix things by adding meta tags manually which should work on both ios and android.
Commit:
7fe5098
Also, I moved src/images/react_logo.png
to static/logo-512x512.png
and added static/logo-180x180.png
for apple.
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.
Okay. This seems to work better in iOS+Safari and Android/Chrome now.
background_color: '#20232a', | ||
theme_color: '#20232a', | ||
display: 'standalone', | ||
icon: 'static/logo-512x512.png', |
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.
What's this icon
actually being used for at this point? Can we remove it, given that you've added the static 180x180 icon and that's the only one referenced in the <link>
?
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.
the one added in <link>
is for apple. Apparently, some versions of apple do not read icons from webapp manifest so they need a special tag in html head but Chrome and other browsers use webapp manifest for icons.
So we'll still need icon
for chrome
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.
So we'll still need
icon
for chrome
Ah, is this the icon shown as the splash screen when the app is opening?
TBH I'm not sure if this change is a net positive for Chrome. It's nice that the default title is just "React", and the splash screen is nice- but the "Add to homescreen" action takes a couple of seconds (during which I see a loading spinner) now instead of just being instant (like it is on reactjs.org, without this change) -
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.
icon
takes that image and generates multiple images so 512x512
is used in homescreen and 192x192
is usually used to create that logo on android and other sizes are for different screen sizes I guess.
I am not sure but I think the reason logo loads fast without icon
is that it uses favicon.ico
which is already loaded when you open website but when you specifically define icon it probably loads when you click 'Add to Homescreen'.
Also, we can't really rely on favicon to provide logo since it does not work in some mobiles (does not work in mine at least)
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 really know much about this particular stuff. I'd love for someone else to weigh in.
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.
Cool, thank you for the quick response :)
Hi @alexkrolick , Can you (or request someone to) review this please? thanks. |
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 guess this is fine. I don't think anyone else is going to look into it any time soon. We can always revert it if it causes a problem.
This is so cool! thank you so much 🌻 |
Hey! There is one plugin missing for this to work properly as a PWA. I've added it: I went to the React website and looked for the little + install button in the Omnibox and it wasn't there. Adding this should make it work as it should. @saurabhdaware thanks for doing this! I wish I had thought of it. 😄 |
Hi @twhite96 Since it was my first PR I wanted to keep things simple so I didn't add it but yes , it would be really cool to have service worker to make it work offline! Good luck with the PR :D |
This solves issue #2259
Then
Currently there is not manifest set for the website so logo is blank and title is set as a name. Also the color of URL bar in chrome is plain white. Here's how it will look after this PR.
Now
Image 1 is to show the new color of URL bar in chrome
Image 2 is the screen after selecting "Add to homescreen"
Image 3 is the splash screen after opening it as WebApp
I've added color to the URL bar, Configured name, short_name and other properties. and added a logo file in
src/images/react_logo.png
(Needed 512px png logo so had to add a new logo)This is my first contribution here so I'm sorry if I missed out on something