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

WebApp manifest added to allow users "Add to Homescreen" documentations #2377

Merged
merged 5 commits into from Oct 9, 2019
Merged

WebApp manifest added to allow users "Add to Homescreen" documentations #2377

merged 5 commits into from Oct 9, 2019

Conversation

saurabhdaware
Copy link
Contributor

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

@facebook-github-bot
Copy link
Collaborator

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!

@reactjs-bot
Copy link

reactjs-bot commented Sep 30, 2019

Deploy preview for reactjs ready!

Built with commit 7fe5098

https://deploy-preview-2377--reactjs.netlify.com

@facebook-github-bot
Copy link
Collaborator

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',
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using plugin gatsby-plugin-manifest so the plugin is handling that path.

image

So it is simply taking that image and generating required variant sizes before it builds. As you can see in the screenshot it generated those images.

Copy link
Contributor

@bvaughn bvaughn Sep 30, 2019

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@bvaughn bvaughn left a 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',
Copy link
Contributor

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>?

Copy link
Contributor Author

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

Copy link
Contributor

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) -

Screenshot_20190930-140116_Chrome

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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 :)

@saurabhdaware saurabhdaware changed the title Closes #2259 WebApp manifest added to allow users "Add to Homescreen" documentations WebApp manifest added to allow users "Add to Homescreen" documentations Oct 1, 2019
@saurabhdaware
Copy link
Contributor Author

Hi @alexkrolick , Can you (or request someone to) review this please? thanks.

Copy link
Contributor

@bvaughn bvaughn left a 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.

@bvaughn bvaughn merged commit 0726327 into reactjs:master Oct 9, 2019
@saurabhdaware
Copy link
Contributor Author

This is so cool! thank you so much 🌻

@twhite96
Copy link

Hey! There is one plugin missing for this to work properly as a PWA. I've added it: gatsby-plugin-offline. Without it, you cannot install the PWA on Chrome desktop or iOS, etc.

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. 😄

@saurabhdaware
Copy link
Contributor Author

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

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.

None yet

5 participants