-
Notifications
You must be signed in to change notification settings - Fork 99
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 UI to select an offline page #28
Conversation
Mainly taken from the privacy policy page UI in wp-admin/privacy.php. Some items remain, including adding the custom post status, and adding the href value.
public function settings_field() { | ||
add_settings_field( | ||
self::SETTING_ID, | ||
__( 'Progressive Web App Offline Page', 'pwa' ), |
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.
Instead of “Progressive Web App Offline Page” I think this should be something like:
Page displays when offline
This would align more with the existing:
Your homepage displays
Then there can be a p.description
element which explains that this is is for PWA, and how provide more details about how it works, maybe mentioning it is “like a 404 page but it shows when someone's internet is down (e.g. during flight or going through tunnel), as opposed to when content is not found.”
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.
@westonruter Is this what you are thinking?
So far, this PR doesn't actually serve the offline page. It only saves the post ID as an option, with the name of WP_Offline_Page::OPTION_NAME. This should also serve the page, per #23. |
That's fine. I think it's good to work on the UX for selecting the offline page separately from the logic from actually serving the offline page. The serving of the offline page should probably incorporate Workbox (#5) and also it will probably involve precaching (#25) since we'll need to make sure the offline page and its required assets get added to the cache. |
New option name is consistent with the naming structure for static special pages.
Refactored to move the `add_settings_error` code to a separate method. Hooked that method to `admin_notices` action. Now settings errors appear both when saving and upon first loading the Reading page. Modified the "in the trash" error to link a link to the Page's Trash UI.
This new strategy works for both the classic and Gutenberg editors.
Instead of registering multiple callbacks to the `admin_init`, this commit registers `init_admin()`. Then the new callback method handles the order of calling each of the tasks. This commit is more performant by O(n-1): 1. Decreases the processing time to do `add_action` as it only have to invoke it once. 2. Decreases the processing time when `do_action` as it only has to invoke one callback. It's more readable, as we can quickly read that the callback intent is to initialize the admin.
This comment handles excluding special static pages ( i.e. `page_on_front`, `page_for_posts`, and `wp_page_for_privacy_policy`, and `page_for_offline` ) from `wp_dropdown_pages`: 1. When selected, the offline page is excluded from the other dropdowns, preventing it from being selected as anything else but the offline page. 2. The homepage, posts page, and privacy page are all excluded from the Offline Page's dropdown. This commit also includes slight performance improvements by: 1. Initializing static pages on init. 2. Caching the offline page ID within the object itself, while also providing for a hard request (i.e. when true, run `get_option`).
Using the Privacy code, this commit adds the HTML and create new page functionality. A unique action is added to the Reading URL. Upon clicking the "create new page" link, the new functionality inserts a new page and assigns it as the Offline Page.
Offline Page Hi @hellofromtonya, Also, the "create a new one" link works well: |
At this point, we have the following functionality in place with much more to do. Create New Offline PageThe
Tagging the Offline Page in the List TableExclude Static Pages from SelectionAs @kienstra showed above, the new changes exclude static pages from the page pick dropdowns.
Error MessagingError handling occurs on:
Currently errors include:
|
To be consistent with the other special pages (homepage, posts page, and privacy), this commit re-enables the page attributes.
This commit excludes the offline page from all search and the menu builders including Appearance > Menus and Customizer's Menus.
This commit excludes the offline page from rendering on the frontend. A 404 is served up. When using plain permalinks, e.g. https://example.com?page_id=28, it will find and render the offline page. This is an incompletion strategy per conversations in Issue #23 @see #23 (comment).
Grouped the tasks and split into separate classes, as the previous class design was doing too much.
This commit improves the excluder by: 1. Using parse_query. 2. Using $query->is_admin. 3. Checking for the offline page's query first. If yes, setting a 404 on the frontend.
} | ||
|
||
$page_id = wp_insert_post( | ||
array( |
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.
The array()
needs to be wrapped in wp_slash()
, unfortunately.
|
||
if ( ! is_wp_error( $page_id ) ) { | ||
update_option( WP_Offline_Page::OPTION_NAME, $page_id ); | ||
if ( wp_redirect( admin_url( 'post.php?post=' . $page_id . '&action=edit' ) ) ) { |
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 don't think think using wp_redirect()
in the conditional is right, since it will only return false if you don't provide a location, which one is being provided. Also, it should be wp_safe_redirect()
for good measure.
|
||
if ( ! is_wp_error( $page_id ) ) { | ||
update_option( WP_Offline_Page::OPTION_NAME, $page_id ); | ||
if ( wp_redirect( admin_url( 'post.php?post=' . $page_id . '&action=edit' ) ) ) { |
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.
This should use get_edit_post_link()
instead of manually constructing the URL.
|
||
$page_id = wp_insert_post( | ||
array( | ||
'post_title' => __( 'Offline Page', 'pwa' ), |
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.
Since this title is going to be displayed to the end user, it should just be “Offline”.
*/ | ||
public function add_post_state( array $post_states, $post ) { | ||
if ( $this->manager->get_offline_page_id() === $post->ID ) { | ||
$post_states[] = __( 'Offline Page', 'pwa' ); |
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.
Excellent
* @return array | ||
*/ | ||
public function get_static_pages( $hard = false ) { | ||
if ( ! $hard ) { |
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.
Since these options are autoloaded, they will aways be loaded from the cache and not from the DB. So there is no need to add another cache.
protected $static_pages = array( | ||
'page_on_front' => 0, | ||
'page_for_posts' => 0, | ||
'wp_page_for_privacy_policy' => 0, |
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.
Shouldn't this be the same as self::OPTION_NAME
?
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.
@westonruter This one is for the privacy page and not the offline page. It's used to exclude each of the special pages from the Offline Page's select dropdown.
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.
Your right. I totally missed this.
// Check that it excludes the configured static pages. | ||
update_option( 'page_on_front', (int) $page_ids[0] ); | ||
update_option( 'page_for_posts', (int) $page_ids[1] ); | ||
update_option( 'wp_page_for_privacy_policy', (int) $page_ids[2] ); |
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.
This string should instead be \WP_Offline_Page::OPTION_NAME
.
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.
Privacy policy page is a special page too. In this test we want to ensure that the page_on_front
, page_for_posts
, and wp_page_for_privacy_policy
are excluded from the offline page dropdown.
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.
@hellofromtonya Oh, my mistake! I mistakenly read wp_page_for_privacy_policy
as wp_page_for_offline_page
or something. I actually hadn't seen the wp_page_for_privacy_policy
option before, so I mistakenly glossed over it.
* | ||
* @return array|string | ||
*/ | ||
protected function get_static_pages_ids( $join = false ) { |
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.
See note below about how $join
isn't necessary since we can just implode( ',', ... )
when calling.
Also, I don't think we need get_static_pages_ids
either. We can just do:
array_filter( $this->manager->get_static_pages() )
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.
Much better refactor. Thank you @westonruter.
'option_none_value' => '0', | ||
'selected' => intval( $this->manager->get_offline_page_id() ), | ||
'post_status' => array( 'draft', 'publish' ), | ||
'exclude' => esc_html( $this->get_static_pages_ids( true ) ), |
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.
This needs to not exclude the offline page, or else it won't be selectable.
/** | ||
* This class manages the Offline Page. | ||
*/ | ||
class WP_Offline_Page { |
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.
This class is pretty small. Would it make more sense to have WP_Offline_Page_UI
and WP_Offline_Page_Excluder
be just integrated into this one class? It could be called WP_Offline_Manager
perhaps.
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 thought about that too @westonruter. But the approach that I took was to separate out by focus and functionality. The WP_Offline_Page
is a manager. You are right that it is a better representation of what it does and the reason it exists.
Why did I break out UI and Excluder from the Manager? Great question @westonruter.
We have these distinct concerns in the Offline Page:
- Routing
- Serving the page
- Excluding the page from queries, searches, dropdowns, etc.
- UI - including user interfaces, settings, workflow, and messaging.
We could combine the serving and excluding concerns into one class, as both are focused on routing and queries. But I think that the UI portion is a separate concern. In time, I'm betting that the UX will grow as we think through how to empower users and make it even easier for them to leverage the plugin.
Where did the manager come from? It serves as a mediator to create the dependent objects, determine when to initialize them, and share commonality between them. It also provides the plugin (later core) bootstrap with one object to create and initialize.
@westonruter What do you think?
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.
That sounds fine with me. We can always revisit later if it makes sense to merge into a single class for the core merge. But for now we can continue as it is.
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.
@westonruter I think we can rename and refocus the excluder
class into a more inclusive responsibility of handing the request whether for serving or excluding, WP_Offline_Page_Request
.
$page_id = wp_insert_post( | ||
wp_slash( array( | ||
'post_title' => __( 'Offline', 'pwa' ), | ||
'post_status' => 'draft', |
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.
As discussed, let's have this create a publish
'ed page. If we allow selecting a draft page then it wouldn't be accessible to the user.
$query->is_singular = false; | ||
$query->set( 'page_id', 0 ); | ||
} elseif ( $this->is_okay_to_exclude( $query ) ) { | ||
$query->set( 'post__not_in', array( $this->manager->get_offline_page_id() ) ); |
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.
The post__not_in
query var is advised against for performance reasons: https://vip.wordpress.com/documentation/performance-improvements-by-removing-usage-of-post__not_in/
Maybe that doesn't apply here.
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.
@westonruter I consider that in my research. Let's talk through it to see if we agree that this use case does not apply or if there is a better way to comply but also be performant.
The Concerns with post__not_in
var
The concerns about the usage of post__not_in
var include:
the query cache hitrate will often be a lot lower
True. But in this use case, we do not want the offline page included the caching as it should not be included or rendered on the frontend.
make the query slower if the exclusion list is large.
Notice that slower occurs when the list is large. In this use case, our list consists of one (1) item, i.e. one page ID.
The VIP Proposed Solution - Iterate.
In almost all cases you can gain great speed improvements by requesting more posts and skipping the posts in PHP.
The proposed alternative solution then is to include the offline page in the query and then remove it or skip over it when iterating through the collection of posts.
In our use case, this solution would require us to provide the means to exclude the offline page from the collection of posts after we get it back from the database. Let's think about what this means.
-
The offline page would be cached.
That's not desirable in our use case. -
We'd need to remove it from the array of posts.
We could hook into one of the posts filters, such asposts_results
. However, iterating over the collection to remove one item is not performant either.
My Thoughts
I think our use case is the perfect use case for post__not_in
. Why?
- PWA is proposed to be merged into core.
- We need to exclude the offline page ID from all frontend queries and all searches.
- It's more performant to exclude in the SQL query than to iterate over the collection of posts for removal.
- We do not want the offline page to ever be in the cache.
IMO I think using post__not_in
is the best strategy overall for this use case.
@westonruter What do you think? Are there other ways we could explore for excluding it from the query that do not involve using post__not_in
?
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.
Your reasoning resonates with me! ✅
$query->is_404 = true; | ||
$query->is_page = false; | ||
$query->is_singular = false; | ||
$query->set( 'page_id', 0 ); |
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.
Are these being done in anticipation of there being an is_offline
?
I don't think that we should necessarily make these changes here. The offline page is a page, so is_page()
should return true
. But what we could do here is $query->is_offline = $this->is_offline_page_query( $query )
to dynamically define a new member variable that can be looked at similarly to the others. We can then introduce a new global function:
function is_offline() {
global $wp_query;
if ( ! isset( $wp_query ) ) {
_doing_it_wrong( __FUNCTION__, __( 'Conditional query tags do not work before the query is run. Before then, they always return false.' ), '3.1.0' );
return false;
}
return ! empty( $wp_query->is_offline );
}
And this is_offline()
function can be used in the template to conditionally show content for offline responses.
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.
Are these being done in anticipation of there being an is_offline?
@westonruter No, it's meant to prevent the offline page from being displayed on the frontend. It's forcing a 404. It was added to provide the functionality discussed here in Issue #23.
I don't think that we should necessarily make these changes here.
Okay. Let's think about this. Maybe we don't care if the default offline page is requestable or viewable on the frontend.
- We're already excluding it from the query via
post__not_in
. - We exclude it from the menus.
What's the harm of viewing it on the frontend? If no or little, then there's no need to set a 404 here.
And this is_offline() function can be used in the template to conditionally show content for offline responses.
Agreed. I was thinking about adding that functionality in a separate PR. We'll want pages/content be marked as offline
and then set a var to note it.
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 think there is no need to prevent the offline page from being accessible if someone knows the URL. If the user can access it directly by the URL, that is fine. In fact, that should be actually desirable because it would allow for someone to easily preview their offline page and make changes. The main thing is just that we don't expose the offline page in menus, page lists, searches, etc. In case someone links to the offline page then we could add wp_no_robots()
to run in wp_head
for that specific page.
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.
Bam, done in commit ab4cbc6. The default offline page will now render on the frontend. That makes it easier.
In case someone links to the offline page then we could add wp_no_robots() to run in wp_head for that specific page.
Included in this commit 9669026.
return false; | ||
} | ||
|
||
return ( $this->manager->get_offline_page_id() === $query->get( 'get_id' ) ); |
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.
Is get_id
right here?
I think this method should largely just do:
return $query->is_singular() && $this->manager->get_offline_page_id() === (int) $query->get_queried_object_id();
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.
@westonruter It is checking is_singular
above in the guard clause:
if ( ! $query->is_page || ! $query->is_singular ) {
return false;
}
I'll switch that to $query->is_singular()
instead of $query->is_singular
.
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.
Note that is_singular
implies is_page
, so only one of the conditions is needed.
But I was more drawing attention to $query->get( 'get_id' )
. I haven't seen get_id
before. I don't believe that is right, and get_queried_object_id()
is what you intend?
'show_option_none' => sprintf( esc_html__( '%1$s Select %1$s', 'pwa' ), '—' ), | ||
'option_none_value' => '0', | ||
'selected' => intval( $this->manager->get_offline_page_id() ), | ||
'post_status' => array( 'draft', 'publish' ), |
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.
Per above, let's only include publish
here.
|
||
wp_dropdown_pages( | ||
array( | ||
'name' => esc_html( WP_Offline_Page::OPTION_NAME ), |
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.
Should actually be esc_attr()
.
$query = new WP_Query( array( | ||
'post_type' => 'page', | ||
'posts_per_page' => 1, | ||
'post_status' => array( 'publish', 'draft' ), |
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.
Per above, let's only include publish
here.
$query->is_singular = false; | ||
$query->set( 'page_id', 0 ); | ||
} elseif ( $this->is_okay_to_exclude( $query ) ) { | ||
$query->set( 'post__not_in', array( $this->manager->get_offline_page_id() ) ); |
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.
👉 This is not accounting for whether there is any existing post__not_in
query var set. So at the very least this should merge with any existing value.
We want to include all special pages, as we use this collection to remove these special pages from the page select dropdowns.
…e page dropdown and that the offline page is included and selected.
This change was requested by Alberto. He's right as the previous label could be confusing as more than one page can be served in offline. But this specific page is the default offline status page when the one requested is not available.
return array( | ||
'page_on_front' => (int) get_option( 'page_on_front', 0 ), | ||
'page_for_posts' => (int) get_option( 'page_for_posts', 0 ), | ||
self::OPTION_NAME => $this->get_offline_page_id(), |
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.
The wp_page_for_privacy_policy
needs to be restored here.
@@ -176,8 +176,7 @@ public function test_render_settings() { | |||
// Check that it excludes the configured static pages. | |||
update_option( 'page_on_front', (int) $page_ids[0] ); | |||
update_option( 'page_for_posts', (int) $page_ids[1] ); | |||
update_option( 'wp_page_for_privacy_policy', (int) $page_ids[2] ); | |||
$this->manager->get_static_pages( true ); | |||
update_option( WP_Offline_Page::OPTION_NAME, (int) $page_ids[2] ); |
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 mistakenly equated wp_page_for_privacy_policy
with WP_Offline_Page::OPTION_NAME
here.
Being mindful of performance, this commit checks if the "post__not_in" has a value. If yes, then it merges with the offline page ID and then filters for only the unique values. Else, it sets the offline ID only, thereby skipping the `array_merge` and `array_unique`. The code is more complex. But it serves to only run the the array functions when needed.
return false; | ||
} | ||
|
||
return $this->manager->get_offline_page_id() === (int) $query->get_queried_object_id(); |
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.
Minor thing but we could combine these:
return (
! $query->is_admin
&&
$query->is_singular()
&&
$this->manager->get_offline_page_id() === (int) $query->get_queried_object_id()
);
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 could. I tend to break them out separately. But we can definitely combine.
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.
@westonruter Changes added in commit af9aedd.
*/ | ||
public function exclude_from_query( WP_Query $query ) { | ||
if ( $this->is_offline_page_query( $query ) ) { | ||
add_action( 'wp_head', 'wp_no_robots' ); |
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 don't think this is quite right because it could result in wp_no_robots
being added unintentionally. Consider, for example, a request to a normal post on a site which at init
or some other early hook does (for some reason):
$offline_query = new WP_Query( array(
'page_id' => get_option( WP_Offline_Page::OPTION_NAME )
) );
This will result in the page getting wp_no_robots
added since the parse_query
action runs for every instance of WP_Query
whether it is the main query or not.
It would be better to separate the concerns here to add a new method like:
function show_no_robots_on_offline_page() {
global $wp_query;
if ( $this->is_offline_page_query( $wp_query ) ) {
wp_no_robots();
}
}
And then add this method to the wp_head
action unconditionally in WP_Offline_Page_Excluder::init()
.
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.
It is giving a side effect. I was trying to avoid hooking in again and rerunning the check. But I do like the separation. Agree on your points.
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.
@westonruter Changes added in commit af9aedd.
Update: The state of the UI as merged in this PR is as follows:
The subsequent screenshots are snapshots during development.
WIP Pull Request
wp-admin/privacy.php
.Closes #23
There's no "Use This Page" element as shown in Issue 23 (comment).
That's an
<input type="submit">
, which submits the<form>
. The entire "Reading Settings" page is a form, which should probably only submit with the "Save Changes" element at the bottom.If you try to save a post that doesn't exist (error mainly taken from the Privacy Page UI):
If the post is in the trash (also mainly taken from
wp-admin/privacy.php
: