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

Show link to exit mobile version on AMP pages even when mobile redirection is not enabled #5293

Closed
Tracked by #7268
westonruter opened this issue Aug 28, 2020 · 11 comments · Fixed by #7426
Closed
Tracked by #7268
Labels
Changelogged Whether the issue/PR has been added to release notes. Groomed Needs sizing P2 Low priority Punted Reader Mode WS:Core Work stream for Plugin core
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Aug 28, 2020

Feature description

In versions of the plugin prior to 2.0, there was an “Exit Reader Mode” link that could be displayed in the header. We removed this in #4573 because it conflicted with long site titles. We also deemed it redundant with the new Mobile Redirection functionality which shows such an exit link in the footer.

However, the exit mobile version only displays in the footer when Mobile Redirection is enabled. We should consider restoring the exit mobile version link when Mobile Redirection is disabled, especially when a site is in Reader mode. The link is probably not needed in Transitional mode because the AMP and non-AMP versions have much more parity. In Reader mode, however, the AMP version is intentionally streamlined so it makes sense that a user seeing the Reader version would always want to leave to go to the non-AMP version.

When mobile redirection is turned off, the link to return to the mobile version would not correspondingly be added to the non-AMP page.

This may require changing the Mobile Redirection section to instead be Paired Options, which could then incorporate the Paired URL Structure section.

More context: https://wordpress.org/support/topic/amp-2-0-exit-reader-mode/#post-13324157


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Reader Mode WS:Core Work stream for Plugin core labels Aug 28, 2020
@westonruter westonruter added this to the v2.1 milestone Aug 28, 2020
@westonruter
Copy link
Member Author

I milestoned this for 2.1 but we can do it in a patch release as well.

@westonruter
Copy link
Member Author

Also apparently requested in support topic:

Also, if the reader mode is enabled I think we can give a link for the user with a button to redirect them to original page?

@westonruter
Copy link
Member Author

When mobile redirection is turned off, the link to return to the mobile version would not correspondingly be added to the non-AMP page.

Or maybe users should have the option to show the mobile switcher link even on non-AMP pages, but this would be less commonly needed than showing the non-AMP link on AMP pages.

@thelovekesh
Copy link
Collaborator

@westonruter While working on #7288 we consider to fix this but due to some reason, we didn't move forward with it which is mentioned here - #7288 (comment)

If this is something we don't want to incorporate based on the above discussion I think we can close this ticket or change the milestone accordingly.

@westonruter
Copy link
Member Author

we didn't move forward with it which is mentioned here - #7288 (comment)

@thelovekesh I'm not sure that is the reason? I think all that is needed to close this out would be to add something like the following if statement to MobileRedirection::register():

if ( ! $is_mobile_redirect_enabled && ! amp_is_canonical() ) {
   add_action( 'template_redirect', function () {
       if ( amp_is_request() ) {
           $this->add_mobile_switcher_footer_hooks();
       }
   } );
}

@thelovekesh
Copy link
Collaborator

@westonruter Sorry I missed #7288 (comment) to include. Can you take a look once at this discussion as well?

@westonruter
Copy link
Member Author

westonruter commented Jan 24, 2023

There are three links involved:

  1. The link added to the head on non-AMP pages to indicate where the mobile (AMP) version is.
  2. The hyperlink (a) added to the footer on AMP pages to allow the user to go back to the non-AMP version.
  3. The hyperlink (a) added to the footer on non-AMP pages to allow the user to go back to the AMP version if they had previously clicked the non-AMP link on mobile (the previous point), which disables the automatic redirect to the AMP version. This link is only shown if using a mobile user agent, and this requires JS.

Point 1 is already implemented in #7288. Point 3 is what I think that comment is mostly about. What we need is point 2 which is to show the non-AMP link on AMP pages when mobile redirection is not enabled.

@pavanpatil1
Copy link

QA Passed ✅

Cross-checked the issue and the fix is working as expected. Now the exit mobile version link is visible even when the mobile redirection option is disable.

mobileexitbtn.mp4

@thelovekesh
Copy link
Collaborator

@westonruter Just noticed that this fix also outputs the Exit mobile version in transitional mode as well which should not be allowed. The logic here should be updated to:

public function maybe_add_mobile_switcher_link() {
if ( amp_is_request() ) {
$this->add_mobile_switcher_head_hooks();
$this->add_mobile_switcher_footer_hooks();
}
}

public function maybe_add_mobile_switcher_link() { 
-	if ( amp_is_request() ) { 
+	if ( amp_is_request() && AMP_Theme_Support::READER_MODE_SLUG === get_option( Option::THEME_SUPPORT ) ) { 
		$this->add_mobile_switcher_head_hooks(); 
		$this->add_mobile_switcher_footer_hooks(); 
	} 
} 

@westonruter
Copy link
Member Author

@thelovekesh Good catch. Please make it so.

@thelovekesh
Copy link
Collaborator

QA Passed ✅

We are aiming to show Exit Mobile Version link to the visitor in transitional mode as well. See: #7453 (comment)

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Groomed Needs sizing P2 Low priority Punted Reader Mode WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants