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

PWA: Add 'offline.php' template #523

Merged
merged 5 commits into from
Oct 24, 2019
Merged

Conversation

laurelfulford
Copy link
Contributor

@laurelfulford laurelfulford commented Oct 22, 2019

All Submissions:

Changes proposed in this Pull Request:

This PR is a first pass at adding a customized PWA error template to the theme.

It should use the theme's branding, and display a PWA error message, like:

IMG_9259

On top of the actual code, I'd be interested to hear feedback about what elements actually make sense to have in this template. Right now, for example, it loads the menu, footer widgets, and ads. Would it be better to strip that kind of stuff out?

Once the template itself looks okay, we can look at jazzing it up a bit (like maybe adding headlines for the five most recently published articles?).

@westonruter I would especially appreciate your expertise on that part!

Closes #357.

How to test the changes in this Pull Request:

  1. Apply the PR and run npm run build.
  2. Make sure the PWA plugin is installed and enabled.
  3. Test the presence of the PWA error message in one of two ways:

a) You can add ?wp_error_template=offline to the URL, to see the template and page title (but not actual error message).

OR

a) On a mobile device, view the site and add it to the Home Screen.
b) Switch the phone to airplane mode.
c) Try loading the site from the link on the homepage; the initial page you saved should still work, but if you click another link that hasn't been saved, you should see the error message.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?

@laurelfulford laurelfulford added the [Status] Needs Review The issue or pull request needs to be reviewed label Oct 22, 2019
@amedina
Copy link

amedina commented Oct 22, 2019

Is the red color part of the theme or is it being used only for the error page?

If the latter is the case, you could experiment with a color/design that may be aligned with the theme palette, so that the error condition is experienced more gracefully by the user. One example I love is the 404 error page in slack.com. It does not feel as an error condition.

@laurelfulford
Copy link
Contributor Author

Is the red color part of the theme or is it being used only for the error page?

Ah, that was not a great choice for that screenshot, but it was coming from the settings I had on my test site (a solid background on the header, plus a very red primary colour).

I've attached a couple more screenshots using different colours/settings, to get a better feel of what it would actually look like with different settings:

IMG_9261

IMG_9260

Copy link
Contributor

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

The code looks good! Compare with an offline.php from Twenty Seventeen: https://github.com/westonruter/twentyseventeen-westonson/blob/master/offline.php

On top of the actual code, I'd be interested to hear feedback about what elements actually make sense to have in this template. Right now, for example, it loads the menu, footer widgets, and ads. Would it be better to strip that kind of stuff out?

If offline, you may consider removing the sidebar widgets and nav menus, yeah. You can see in the example above that this was done via:

// Prevent showing nav menus.
add_filter( 'has_nav_menu', '__return_false' );

Once the template itself looks okay, we can look at jazzing it up a bit (like maybe adding headlines for the five most recently published articles?).

Then once the PWA plugin facilitates listing the offline pages available (GoogleChromeLabs/pwa-wp#211), these can be added to the content of the template, allowing the user to navigate to those previously-cached URLs which we know are available (as opposed to the nav menu and widget links which may not be).

offline.php Outdated Show resolved Hide resolved
@laurelfulford
Copy link
Contributor Author

Thanks @westonruter!

I've updated the template to remove menus and widgets in 48f8181, as well as WordAds; I need to do a little more work to get the ads added by the Newspack Plugin also removed.

Then once the PWA plugin facilitates listing the offline pages available (GoogleChromeLabs/pwa-wp#211), these can be added to the content of the template, allowing the user to navigate to those previously-cached URLs which we know are available (as opposed to the nav menu and widget links which may not be).

Right! Yes, that'd be a lot more useful :)

@jeffersonrabb jeffersonrabb self-requested a review October 24, 2019 11:55
Copy link
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

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

Looks good, worked well in testing on a real mobile device.

I did see an Ad placeholder at the bottom of the offline template, as shown here. I assume this is the following, which will be addressed separately elsewhere?

I need to do a little more work to get the ads added by the Newspack Plugin also removed.

Screenshot 2019-10-24 at 08 42 14

@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Oct 24, 2019
@laurelfulford
Copy link
Contributor Author

Thanks @jeffersonrabb!

I assume this is the following, which will be addressed separately elsewhere?

I actually meant to continue working on it in this PR, but I didn't make that very clear! I pushed up what I had until I could get remove_action working with the ads coming from Newspack.

Since this is approved, I can merge this PR and work on that in a separate one.

@laurelfulford laurelfulford merged commit f961fce into master Oct 24, 2019
@laurelfulford laurelfulford deleted the build/offline-template branch October 24, 2019 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PWA error template
4 participants