-
-
Notifications
You must be signed in to change notification settings - Fork 301
Add Rails PWA support #2141
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
base: main
Are you sure you want to change the base?
Add Rails PWA support #2141
Conversation
zackgilbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment inline. I'm okay with this (it's a step closer), but think we can go a step further that actually enables them so a project could be added out of the box.
| <%= capybara_lockstep if defined?(Capybara::Lockstep) %> | ||
|
|
||
| <%# Enable PWA manifest for installable apps (make sure to enable in config/routes.rb too!) %> | ||
| <%#= tag.link rel: "manifest", href: pwa_manifest_path(format: :json) %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you wouldn't just enable this by default?
| "name": "<%= t('application.name') %>", | ||
| "icons": [ | ||
| { | ||
| "src": "/heroku-logo.png", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd want to use our app's logo, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the starter repo this is the app logo. Is there some other file you think we should point at by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Didn't realize. Was thinking it was the logo in the assets folder or something like that. No worries then, if that's the main logo. I thought it was the heroku logo, which didn't make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that files has a weird name for some reason...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we do seem to have several copies of more-or-less the same logo scattered around the starter repo. Might be worth consolidating and cleaning up.
| extending = {only: []} | ||
|
|
||
| # Render dynamic PWA files from app/views/pwa/* (remember to link manifest in layouts/_head.html.erb) | ||
| # get "manifest" => "rails/pwa#manifest", as: :pwa_manifest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like these (this and service-worker on the next line) could be enabled as well, no? Any reason not to?
pascallaliberte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like a bullet_train thing to do would be to have a pwa_enabled? helper method returning true if ENABLE_PWA=true env var is set.
What do you all think?
|
@zackgilbert As far as whether or not to enable these things by default I have a few kind of related thoughts:
All that said, I did go back and forth over whether we should enable this stuff by default, and the reasons listed above just tilted me towards leaving them disabled. I'm happy to consider reasons for enabling them by default. |
|
@pascallaliberte having some configs was one of my first considerations: #2100 (comment) Might be the way to go... |
|
If we did go with the config/helper method route we could move the routes and the layout bits into |
|
How about manifest.json.example? And instead of an ENV var, we just check the presence of the manifest.json file? Solves the file overwriting, allows us to stuff those routes in gems, we could have smart defaults in that file (logo path), and to make it super clear to partial ejectors, the helper could be named |
🤔 Interesting idea... |
This adds support for Rails Progressive Web App feature which was added in Rails 7.2.
Fixes: #2100
This basically adds the same starter bits that are generated with a new Rails application.
/manifest.jsonso that an app can be "installed" locally/service-worker.jsapp/views/layouts/_head.html.erbfor pointing browsers at/manifest.jsonconfig/routes.rbfor serving/manifest.jsonand/service-worker.jsTo make use of these you'll need to uncomment things as appropriate and customize them to your needs.
NOTE: To make use of the Service Worker you need to register it somehow via your own app Javascript. It's up to you to decide if you want to register it automatically or prompt users to opt-in.
After uncommenting the lines for enabling the manifest you should see some way to install the app (in browsers that support installation of PWAs). This is what it looks like in Chrome:
After clicking the button you'll be prompted to install the app:
And then it will be opened in a new instance of the browser that does not have the normal browser chrome but just a header bar for your app:
On MacOS your app will show up in Launchpad: