Conversation
There was a problem hiding this comment.
Minor drive-by nitpick. This, and the other files below, don't end with an empty newline.
There was a problem hiding this comment.
Fixed, thanks. Will push up this change soon.
|
Do you think you could add some screenshots and some testing instructions so we all know what to look for? Thanks! |
jeherve
left a comment
There was a problem hiding this comment.
A few questions as I'm not really familiar with what the module really does just yet.
modules/pwa/pwa.php
Outdated
There was a problem hiding this comment.
How does this play with other AMP plugins like our own? Shouldn't we not output anything when that plugin is active?
There was a problem hiding this comment.
amp_post_template_head is an action fired by our AMP plugin. AMP needs a specific manifest format that is not regular HTML. This feature is designed to work with the official AMP plugin, not replace it.
modules/pwa/pwa.php
Outdated
There was a problem hiding this comment.
How does this work on sites that do not have an active SSL certificate?
There was a problem hiding this comment.
It doesn't work at all on those sites.
I will soon! This is still a work in progress. |
| 'jp_group' => 'pwa', | ||
| ), | ||
| 'pwa_lazy_images' => array( | ||
| 'description' => esc_html__( 'Display notice on page when the browser is offline', 'jetpack' ), |
| return cache.match( request ) | ||
| .then( function( response ) { | ||
| // only do more work if we don't already have a response | ||
| if ( response ) { |
There was a problem hiding this comment.
What's the invalidation process? If visitor caches content and site owner updates the resource.. Unlike the "regular" HTTP cache, resources in SW cache are not automatically checked for freshness and have to be manually (re+in)validated by our code here.
There was a problem hiding this comment.
I added stale-while-revalidate in the latest couple of commits. I'm not actually taking over the browser and replacing content yet, that's next...
| return isset( $registration->extra['jetpack-inline'] ) && $registration->extra['jetpack-inline']; | ||
| } | ||
|
|
||
| private function should_remove_style( $handle ) { |
There was a problem hiding this comment.
For locally defined (inlined fonts), a better solution might be to inject a font-display [1] swap? For external resources, like Typekit and Google Fonts, that won't work today, but hopefully soon, once implementations for [2] land in Chrome and other browsers.
Alternatively, have you considered non-blocking font load / loadCSS [3] variant instead? That introduces FOUT, but I still think it's better than omitting them entirely?
[1] https://www.w3.org/TR/css-fonts-4/#font-display-desc
[2] https://www.w3.org/TR/css-fonts-4/#font-display-font-feature-values
[3] https://github.com/filamentgroup/loadCSS#recommended-loadcss-usage
|
Is there any particular reason for using protocol relative and not HTTPS URLs when connecting to CDNs? There is no reason not to make the CDN URL an HTTPS one |
| } | ||
|
|
||
| private function get_service_worker_url() { | ||
| return add_query_arg( PWA_SW_QUERY_VAR, '1', trailingslashit( site_url() ) . 'index.php' ); |
There was a problem hiding this comment.
If pretty permalinks are enabled, shouldn't this return home_url( '/service-worker.js' ) as the rewrite rule was added in the register_rewrite_rule method?
|
@gravityrail can this be closed? |
Adds some basic PWA features: