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

Add manifest fields for PWA install bottom sheet #933

Merged
merged 8 commits into from
Jan 21, 2021

Conversation

beaufortfrancois
Copy link
Contributor

This PR adds standardized manifest fields for PWA install bottom sheet such as description, lang, categories, and screenshots so that the PWA install bottom sheet looks great. See screenshot below.

Note that it currently requires chrome://flags/#mobile-pwa-install-use-bottom-sheet flag in Chrome Canary on Android.

image

description:
'Compress and compare images with different codecs, right in your browser.',
lang: 'en',
categories: ['photo', 'productivity', 'utilities'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

@beaufortfrancois are these from a particular list of categories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@google-cla
Copy link

google-cla bot commented Jan 15, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed CLA: Yes labels Jan 15, 2021
@jakearchibald
Copy link
Collaborator

@googlebot I consent.

@google-cla google-cla bot added CLA: Yes and removed cla: no labels Jan 15, 2021
@jakearchibald
Copy link
Collaborator

@beaufortfrancois I've switch the images to webp so they're around a 10th of the size. Is webp ok to use here?

@beaufortfrancois
Copy link
Contributor Author

WebP is still not supported on Android sadly. See https://source.chromium.org/chromium/chromium/src/+/master:components/webapps/browser/installable/installable_manager.cc;l=113;bpv=1;bpt=1?q=fetchScreenshot&ss=chromium and https://bugs.chromium.org/p/chromium/issues/detail?id=466958
It is supported on Desktop though but this bottom sheet is only for Android, hence why I've used png there.
We can try to reduce image dimension if you want.

@jakearchibald jakearchibald requested a review from surma January 15, 2021 13:39
@jakearchibald
Copy link
Collaborator

@surma could you do a review of this? I've added quite a bit to it


const defaultOpts = {
prefix: 'url',
imagePrefix: 'img-url',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cheeky suggestion: Wouldn’t it keep this plugin simpler if instead of a second prefix it would take a list of file extensions that we consider images?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that would make the typing a lot harder. RIght now the module definition is based on prefixes. What you're proposing would depend on both a prefix and a (set of) suffixes, and I don't think TS supports that.

I'll keep it explicit for now, but if the typing can be done, happy for it to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants