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

Improve PWA manifest #2060

Merged
merged 8 commits into from
Oct 8, 2018
Merged

Improve PWA manifest #2060

merged 8 commits into from
Oct 8, 2018

Conversation

Richienb
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Pull requests should be thought of as a conversation. There will be some back and forth when trying to get code merged into this or any other project. With all but the simplest changes you can and should expect that the maintainers of the project will request changes to your code. Please be aware of that and check in after you open your PR in order to get your code merged in cleanly.

Thanks!

@coliff
Copy link
Member

coliff commented Aug 30, 2018

hey there! thanks for the PR. I think you're right that the manifest we include could be improved - but I'm not sure if all of those should be included by default. I suggest to remove:
"display": "standalone", "scope": "/"
I think the 'display' could be a a few different options so providing 'standalone' as the default might not be a good idea.

Also, the changes you make should also be made in the dist dir. (ideally you should make a build locally and that will be done for you)

@Richienb
Copy link
Contributor Author

I've just fixed these problems in 2 commits.

@coliff
Copy link
Member

coliff commented Aug 30, 2018

Cool - sorry if it wasn't clear but the manifest changes needs to be in both the src and the dist folder.

We need to also need to remove that last comma after theme_color so it's valid JSON - so should look like this:

{
	"short_name": "",
	"name": "",
	"icons": [{
		"src": "icon.png",
		"type": "image/png",
		"sizes": "192x192"
	}],
	"start_url": "/?utm_source=homescreen",
	"background_color": "#fafafa",
	"theme_color": "#fafafa"
}

@Richienb
Copy link
Contributor Author

I've updated the manifests and pulled the latest changes from master.

@roblarsen
Copy link
Member

This looks good to me.

I ask, because the way this project works, this matters: is there any functional difference between defining an empty string as name and short_name and not defining them at all?

@Richienb
Copy link
Contributor Author

Richienb commented Sep 3, 2018

No there isn't.

  • name should mirror the <title> tag in the HTML file.
  • short_name is similar to name but is no longer than 12 characters.

This is a short snippet from the manifest file from my website's repository:

"name": "Richie Bendall's Website",
"short_name": "Richie Web",

@roblarsen
Copy link
Member

Thanks, I ask because, as with anything we publish, this file will be passed around liberally, unchanged. I prefer instructive defaults. Empty attributes work for that. We just need to make sure there's no downside to an empty attribute.

@Richienb
Copy link
Contributor Author

Richienb commented Sep 3, 2018

I see... If the user wants PWAs to be supported, him/her can fill out the fields. Otherwise, it can be left alone and not cause errors in the application.

@roblarsen
Copy link
Member

👍

@coliff coliff added this to the 7.0 milestone Oct 4, 2018
@coliff coliff self-assigned this Oct 4, 2018
@coliff
Copy link
Member

coliff commented Oct 4, 2018

I've been testing this today and found the following:

  • adding the name and short-name attributes with empty strings gives errors in Google Lighthouse's PWA test. I don't necessarily think this is a bad thing from our perspective - we also get an error that the HTML title is empty - it just means that users need to enter the sites details.
  • sorry @Richienb - I previously suggested you to remove "display": "standalone" and that is required so I think we should re-add.
  • We get an error that an icon of at least 512px x 512px is not provided. I think we should add one in a different PR. REF: https://developers.google.com/web/tools/lighthouse/audits/custom-splash-screen

@Richienb
Copy link
Contributor Author

Richienb commented Oct 4, 2018

Ok, I'll look into that and make necessary changes

@coliff
Copy link
Member

coliff commented Oct 4, 2018

Hi @Richienb - please feel free to re-add the "display": "standalone" but don't make any other changes for now - let's see what @roblarsen says about the empty strings for name and short-name.
If we add a new icon size that will need to be added in a different PR which requires a few other changes to tests etc.

@Richienb
Copy link
Contributor Author

Richienb commented Oct 4, 2018

I've readded the display standalone attribute to both of the site.webmanifest files

@roblarsen
Copy link
Member

@Richienb @coliff I'm fine with an empty string. It's the least bad option. I'd like to add a simple generator at some point that could fill in things like this (as well as the lang attribute,) but for now empty is the least harmful option.

Copy link
Member

@roblarsen roblarsen left a comment

Choose a reason for hiding this comment

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

:shipit:

@Richienb
Copy link
Contributor Author

Richienb commented Oct 6, 2018

@Richienb @coliff I'm fine with an empty string. It's the least bad option. I'd like to add a simple generator at some point that could fill in things like this (as well as the lang attribute,) but for now empty is the least harmful option.

It might be useful to have an API. You would request individual files and include custom fields as queries. For example: api.html5boilerplate.com/index.html?title=My%20Website would return the index.html file with the title prefilled. Then we would merge these generated files into a zip with an URL like api.html5boilerplate.com/merge/index.html?title=My%20Website|site.webmanifest?name=My%20Website&short_name=Website.

I'm not sure if the hosting service currently hosting the site, Dreamhost supports implementing custom APIs but I know that the implementation is possible because Google Fonts and JsDelivr are currently doing something similar to this.

I admit that this idea sounds very ambitious and probably won't work but it certainly sounds nice to have.

@coliff coliff merged commit 35c6157 into h5bp:master Oct 8, 2018
@coliff coliff mentioned this pull request Oct 9, 2018
10 tasks
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