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 to manifest #3799

Merged
merged 2 commits into from
May 11, 2017
Merged

add to manifest #3799

merged 2 commits into from
May 11, 2017

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented May 3, 2017

for #334
even though app complains about being run in mobile web it is possible to use
and in certain scenarios it might be the best option so this will at least improve that

tested both using Lighthouse and my Android M device

Signed-off-by: Michael Telatynski 7t3chguy@gmail.com

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@lukebarnard1 lukebarnard1 self-assigned this May 11, 2017
@@ -1,5 +1,9 @@
{
"name": "Riot",
"name": "Riot - Matrix Client",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced by this. @dbkr, how does this sit with you?

"url": "https://play.google.com/store/apps/details?id=im.vector.alpha",
"id": "im.vector.alpha"
}, {
"platform": "itunes",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish there was a way to indicate that this isn't a known platform by the w3c. (The w3c claims to only know about "play") but I understand that without the platform property, the application isn't processed... Oh well 😀

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented May 11, 2017

I seem to be getting Site cannot be installed: no matching service worker detected. You may need to reload the page, or check that the service worker for the current page also controls the start URL from the manifest when I try adding my local Riot to my Chrome home screen.

Update: I was using the "Add to homescreen" link in dev tools which apparently doesn't do the same as the standard "Add to desktop..." UI.

@t3chguy
Copy link
Member Author

t3chguy commented May 11, 2017

thats strange, definitely worked fine on my Android device

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented May 11, 2017

Apparently the manifest has to be in the root directory: https://abendigo.github.io/2016/07/17/pwa_step_one.html#comment-3194480539

Tried that, didn't work

It did work, see #3799 (comment)

@t3chguy
Copy link
Member Author

t3chguy commented May 11, 2017

Could you check if you have the same issue at: https://riot.ovh/dev

Lighthouse report: https://riot.ovh/dev/lighthouse.html
only complaint it has is lack of a service worker for app-like behaviour
guess the indexedb worker doesn't quite count

@t3chguy
Copy link
Member Author

t3chguy commented May 11, 2017

I guess we could move some of the syncing to a service worker and even get notifications when the webapp isn't open

@lukebarnard1
Copy link
Contributor

I think we'll have to do more work to implement PWA things (actually implementing a service worker - https://codelabs.developers.google.com/codelabs/add-to-home-screen/#5).

I don't want this to block on what to change the name to, so let's stick with just "Riot" and then I'll merge.

@t3chguy
Copy link
Member Author

t3chguy commented May 11, 2017

@lukebarnard1 I changed the full name to match that of the apps' Riot - open team collaboration

@lukebarnard1
Copy link
Contributor

Well in that case LGTM!

@lukebarnard1 lukebarnard1 merged commit 15db686 into element-hq:develop May 11, 2017
@t3chguy
Copy link
Member Author

t3chguy commented May 11, 2017

@lukebarnard1 I hadn't updated the PR to match that of my working directory haha
but the only differences seem indentation so should be fine

actually no I had moved it outward in anticipation of the scope

@lukebarnard1
Copy link
Contributor

I did not think to check indentation. If you can be bothered to make another PR, go for it 😇

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

Successfully merging this pull request may close these issues.

2 participants