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

Extend classic/reader templates with commonly-requested features #2044

Open
westonruter opened this issue Apr 1, 2019 · 16 comments
Open

Extend classic/reader templates with commonly-requested features #2044

westonruter opened this issue Apr 1, 2019 · 16 comments

Comments

@westonruter
Copy link
Member

@westonruter westonruter commented Apr 1, 2019

Now that we plan to rebrand the Classic mode and maintain it as a “Reader” mode (#2034) we should address common requests by users for the classic post templates.

The most common request I believe is to add a nav menu (#1772, #792, #615, #220, #435). Note that if this involves adding a nav menu location, this could be re-used for navigation in AMP Stories. As with AMP Stories, the nav menu should be presented in an amp-sidebar.

What are the other most-commonly requested template features that make sense for us to incorporate into the plugin? One request is a search bar.

@swissspidy

This comment has been minimized.

Copy link
Collaborator

@swissspidy swissspidy commented Apr 2, 2019

Some users also requested filtering all internal links so that they stay in reader mode while navigating.

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Apr 2, 2019

For that, there is also #1389

@westonruter westonruter added this to the v1.2 milestone Apr 4, 2019
@westonruter westonruter removed this from the v1.2 milestone May 24, 2019
@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Jul 15, 2019

Here is a basic plugin which can be used to add a nav menu sidebar on Reader mode templates: https://gist.github.com/westonruter/b22cfae13f0862d5b9716c776c60f882

@mrjuri

This comment has been minimized.

Copy link

@mrjuri mrjuri commented Jul 16, 2019

Is a good idea make two location menu?

  1. location for AMP page;
  2. location for AMP stories.

So in AMP stories, the menu will be active or not.

@swissspidy

This comment has been minimized.

Copy link
Collaborator

@swissspidy swissspidy commented Jul 16, 2019

@mrjuri Those would be separate things either way eventually, see #2639.

@jeannyhaliman

This comment has been minimized.

Copy link

@jeannyhaliman jeannyhaliman commented Jul 19, 2019

Hi Weston,

I heard similar feedbacks of publishers requesting sidebar & search bar.

Beside that, common asks I've heard:

  1. Feature to upload publisher logo on top of the page. (upon click - redirect to home)
  2. Flexibility of changing the colour of the top bar (currently only available in blue)

Pubs also ask for share to social media, but I usually recommend them to check supported plugins in amp-wp.org/ecosystem

Hope this helps!

Sincerely,
Jeanny

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Jul 23, 2019

@jeannyhaliman

Feature to upload publisher logo on top of the page. (upon click - redirect to home)

Currently the site icon is shown in the upper-right corner of the header.

image

But instead of this they want to have a bigger logo appear instead of the site title in the top-left (here “WordPress Develop”)? Should the site icon then be hidden when that is done?

Flexibility of changing the colour of the top bar (currently only available in blue)

Actually, you can change the color of the header currently:

image

@swissspidy

This comment has been minimized.

Copy link
Collaborator

@swissspidy swissspidy commented Sep 20, 2019

In https://wordpress.org/support/topic/dark-theme-support/ it was suggested to use a prefers-color-scheme media query to automatically switch to dark mode.

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Jan 16, 2020

A couple more things:

  • Implement support for automatically serving mobile visitors to the AMP version, or else redirect them to the AMP version. There would still have to be a way for the user to exit the AMP version. For prototype, see Mobile Redirection plugin.
  • Implement support for non-singular templates, such as the blog index page, category template, author template, and other such archive templates.

In short, these two points are summarized by porting Jetpack's mobile theme into the AMP plugin. This is actually great timing to do this because Jetpack's mobile theme is deprecated and will be removed in March 2020.

Since the mobile theme—Mineleven—to be AMP-compatibile and then bundle it with the AMP plugin. This work is greatly simplified after resolving #2202.

It may also be possible for us to architect it in a way where we allow the theme to be installed as a regular theme and then dynamically switch to it. (A problem here is with bootstrapping and needing to wait for wp action to know if we will be serving an AMP page or not; this difficulty also extends to exposing theme-specific customization options.) This would perhaps allow a user to install alternative AMP-compatibile mobile themes to switch between, as an easier alternative to writing AMP templates and placing them in the active theme's amp directory.

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Jan 30, 2020

Related: #4204 (adding block stylesheets to Reader mode template)

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Jan 31, 2020

I chatted with @gravityrail about Jetpack's Mineleven module (which is deprecated and getting removed). I asked about whether there been headaches about dynamically serving this theme for mobile users. I've suspected the dynamic user-agent detection is somewhat brittle given caching plugins. He confirmed:

We have seen issues with minileven showing the mobile theme under some server and client configurations. Caching plugins, CDNs, caching reverse proxies, varnish and client user-agent spoofing all cause problems.

It's probably possible to solve this with concentrated and long-term effort, so perhaps the WordPress community could come up with a standard function for identifying mobile devices that can live in Core so that we can avoid reinventing the wheel. I'm sure there would be lots of good uses for it. You could use jetpack_is_mobile() as a starting point.

To support serving the AMP templates to mobile visitors we'd have to incorporate the jetpack_is_mobile() logic into the AMP plugin, but this appears to be problematic to get right. We may need to rely on client-side detection of mobile visitors and redirect them via script early in the head, though I am unsure if this is reliable/performant either.

See also Mineleven issues.

@kraftbj

This comment has been minimized.

Copy link
Contributor

@kraftbj kraftbj commented Jan 31, 2020

There is a Core function already ( https://developer.wordpress.org/reference/functions/wp_is_mobile/ ) that we can look at ensuring is up to date for a PHP-based method, but I do agree that if we need a client-side method, we should push for a Core direction to avoid duplication and support headaches with the various differences that a bunch of methods would result in.

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Jan 31, 2020

@kraftbj Yeah. My concern about wp_is_mobile() is that it is quite limited in its detection. The logic in jetpack_is_mobile() seems much more robust. In any case, neither of these is of any help when a caching layer is preventing it from being called in the first place. To use it reliably, caching plugins would need to be told of the user agent variants to vary the caches by.

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Jan 31, 2020

I've put together a proposed approach for how to handle JS-based redirection, which redirects mobile visitors of the desktop site over to the AMP version when it is available. Here's the plugin code: https://gist.github.com/westonruter/5f4f27ed3bdd676e3f08524b9bab8bb4

JS-based redirection is not ideal because of the additional latency, but it seems much more robust (since we don't have to worry about caching plugins or reverse proxy caches). In the approach I took, any resources that start to get loaded on the desktop page get aborted via window.stop():

image

When on the AMP version, any URLs automatically have the have the ?amp query parameter appended to them thank to AMP-to-AMP linking (#1389). A link is added to the footer which links the user to the corresponding non-AMP URL which also causes the redirection to be disabled for the current session.

This works best with a current 1.5-alpha build of the AMP plugin: amp.zip (1.5.0-alpha-20200131T195804Z-caa0c8c61)

Thoughts appreciated!

@gravityrail

This comment has been minimized.

Copy link
Contributor

@gravityrail gravityrail commented Jan 31, 2020

@westonruter

This comment has been minimized.

Copy link
Member Author

@westonruter westonruter commented Feb 6, 2020

As part of the effort there, we need to look at bringing validation error reporting to Reader mode. At present, any validation errors are silently suppressed so users have no idea when something is being removed for being invalid. Note that validation error messages used to be present in Reader mode, but they were removed in #1132 because users were complaining about seeing undesired warnings.

The new Reader mode theme will need to trigger the standard theme hooks including wp_head and wp_footer so that scripts and stylesheets that a plugin may be trying to the page will be included and then reported as validation errors. This would have illuminated the reason for breakages such as ampproject/amphtml#26622.

Of course, there is still going to be the need to suppress validation errors from certain user roles or on certain site configurations (e.g. WordPress.com). This is needed for Standard/Transitional mode already. It would also need to be available in Reader mode. See #2673.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.