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

Added manifest.json, works with no navbar for PWA #6640

Merged
merged 27 commits into from
Feb 22, 2024

Conversation

mw2000
Copy link
Contributor

@mw2000 mw2000 commented Feb 8, 2024

Adds PWA support to the commonwealth mobile website, with a standalone application

Link to Issue

Closes: #6226

Description of Changes

  • Added a manifest.json
  • Added manifest link in top level HTML
  • Added PWA prompt for Android and iOS
  • Added show less feature with exponential backoff for prompt

"How We Fixed It"

Had to find where to place manifest.json file, but everything else was present in the issue.

For prompt, was able to build the custom component for iOS and android and check app status if it was run on browser or as PWA.

Test Plan

  • Start app
  • Open iOS/Android emulator
  • Share PWA to home screen
  • Click on icon and check if standalone or not

Deployment Plan

  • Set installable in manifest.json to false

@mw2000 mw2000 linked an issue Feb 8, 2024 that may be closed by this pull request
@mw2000 mw2000 changed the title Added manifest.json, works on iOS with no navbar for PWA Added manifest.json, works with no navbar for PWA Feb 8, 2024
@CowMuon
Copy link
Contributor

CowMuon commented Feb 9, 2024

It doesn't escape notice that installable key needs to be set to true for development, but needs to be set back to false to push (let's discuss how we handle pushing to frick/frack, since we would need it set back to true.)

@dillchen
Copy link
Contributor

I see zero problem with setting it just to installable now. Seems not worth the back and forth later on, unless I'm missing something

Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

Just realised that it is still in draft, so probably something might not work as expected. But left already a lot of comments regarded to code style etc. Besides,

  1. I am not able to X out from this prompt even though if I add app to the home screen.
  2. On Iphone font is not loaded correctly.
  3. There are differences in the implementations vs designs on both platforms

IMG_4190

IMG_4191

@CowMuon
Copy link
Contributor

CowMuon commented Feb 13, 2024

Really appreciate the code review comments by @masvelio.
@mw2000 Let's focus on attention to detail, some of these code comments should be easily avoidable, kudasai.

@mw2000
Copy link
Contributor Author

mw2000 commented Feb 15, 2024

@masvelio @CowMuon appreciate the detailed review! apologies for what seems to be a depth of errors, but a lot of it were outstanding due to it being a draft PR still under construction. However, I appreciate the guide around some of the code style stuff, since that helps in future PRs as well.

@mw2000 mw2000 linked an issue Feb 15, 2024 that may be closed by this pull request
@mw2000 mw2000 requested a review from masvelio February 15, 2024 03:42
@mw2000 mw2000 marked this pull request as ready for review February 15, 2024 04:28
Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

  1. I can see the prompt on web. I don't think this is expected?

image

  1. How are we gonna detect new version of the app and how to are we gonna force/ask user to update it? Or this is not a case in PWA?

I think it needs one more round of CR. But really appreciate all those changes you did after my previous CR

@mw2000
Copy link
Contributor Author

mw2000 commented Feb 21, 2024

  1. I can see the prompt on web. I don't think this is expected?

image

  1. How are we gonna detect new version of the app and how to are we gonna force/ask user to update it? Or this is not a case in PWA?

I think it needs one more round of CR. But really appreciate all those changes you did after my previous CR

@masvelio so i just tested this out, what happens is when we enter the responsive mode on chrome when already in console mode, the window.navigator.userData matches with Android causing the AndroidPrompt to be shown. I'm not sure how you're getting the IOSPrompt (unless you clicked on iPhone in the size in the console). This is the only way I've seen to reliably detect the OS for the PWA.

@mw2000 mw2000 requested a review from masvelio February 21, 2024 03:11
Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

Did another round of testing, but it is either on localhost using http or ngrok using https. Both are not perfect. Showing videos below from real android device (sorry for quality).

  1. http localhost - our popup appears but I clicking cancel or install does nothing
Screenrecorder-2024-02-21-14-22-53-453.mp4
  1. https ngrok - our popup appears and after clicking install I can see native popup. However, when I click cancel on native, I am not able to perform action again, neither on cancel, nor on install.
Screenrecorder-2024-02-21-14-26-23-359.2.mp4

As you can see it is very wonky. My proposition is that you resolve all my comments regarding the code and we should deploy it to demo/frick/frack and ask other engineers or @ilijabojanovic to test it out. It looks like we might have quite a lot of weird cases here so we need a plan how we want to go about it as probably we don't want to scare away our potential PWA users within first 2 minutes of using the app (or even before installing it). Also we need some strategy from now on how we want to test mobile features because this is already my third run on this PR and each of it was more than one hour of testing and doing code review cc @ForestMars @jnaviask @CowMuon @MuonShot

@jnaviask jnaviask temporarily deployed to commonwealth-frick February 21, 2024 17:29 Inactive
@mw2000
Copy link
Contributor Author

mw2000 commented Feb 21, 2024

@masvelio I was able to fix 2. since that was mainly due to the cancel button breaking in the last commit when I split the component and now install and cancel native prompts.

The only wonky thing I see on my end (on Android) is when i install, then delete the app and then try re-installing it. If i do it within a couple seconds the install button doesn't work. I'm testing on https://frick.commonwealth.im/

This is likely due to the fact that the browser thinks the PWA is still installed, when its not. If I close and create a new tab after this the install and cancel buttons work well.

We currently don't have a viable way to check if the app is installed on the user's phone or not navigator.getInstalledApps doesn't seem to work/gives an empty array. So not sure what solution i should go with for this

@jnaviask jnaviask temporarily deployed to commonwealth-frick February 21, 2024 20:34 Inactive
@jnaviask jnaviask temporarily deployed to commonwealth-frick February 21, 2024 20:48 Inactive
@jnaviask jnaviask temporarily deployed to commonwealth-frick February 21, 2024 21:08 Inactive
@mw2000 mw2000 requested a review from masvelio February 21, 2024 21:31
@jnaviask jnaviask temporarily deployed to commonwealth-frick February 22, 2024 01:01 Inactive
Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

clicked around on android and iphone and after many fixes it works fine for me, so thank you @mw2000 for your hard work!

Copy link
Contributor

@lzach83 lzach83 left a comment

Choose a reason for hiding this comment

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

It works, just a quick code quality fix.

@jnaviask jnaviask temporarily deployed to commonwealth-frick February 22, 2024 17:45 Inactive
@jnaviask jnaviask temporarily deployed to commonwealth-frick February 22, 2024 18:05 Inactive
@mw2000 mw2000 requested a review from lzach83 February 22, 2024 23:07
Copy link
Contributor

@lzach83 lzach83 left a comment

Choose a reason for hiding this comment

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

Good to go

@mw2000 mw2000 merged commit c43fd75 into master Feb 22, 2024
6 of 7 checks passed
@mw2000 mw2000 deleted the 6226-add-manifestjson-for-pwa-standalone-mode branch February 22, 2024 23:14
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.

Enable PWA prompt Add manifest.json for PWA Standalone Mode
6 participants