Skip to content

Improve PWA manifest#2060

Merged
coliff merged 8 commits into
h5bp:masterfrom
Richienb:patch-1
Oct 8, 2018
Merged

Improve PWA manifest#2060
coliff merged 8 commits into
h5bp:masterfrom
Richienb:patch-1

Conversation

@Richienb

Copy link
Copy Markdown
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

coliff commented Aug 30, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

I've just fixed these problems in 2 commits.

@coliff

coliff commented Aug 30, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@roblarsen

Copy link
Copy Markdown
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

Richienb commented Sep 3, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

Richienb commented Sep 3, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

👍

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

coliff commented Oct 4, 2018

Copy link
Copy Markdown
Member

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

Richienb commented Oct 4, 2018

Copy link
Copy Markdown
Contributor Author

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

@coliff

coliff commented Oct 4, 2018

Copy link
Copy Markdown
Member

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

Richienb commented Oct 4, 2018

Copy link
Copy Markdown
Contributor Author

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

@roblarsen

Copy link
Copy Markdown
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.

@roblarsen roblarsen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@Richienb

Richienb commented Oct 6, 2018

Copy link
Copy Markdown
Contributor Author

@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