-
Notifications
You must be signed in to change notification settings - Fork 381
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 paired browsing interface #3656
Conversation
9d49114
to
959d21b
Compare
I've fleshed out the todos based on additional requirements that I hadn't yet added to the corresponding issue. |
This is worth having a conversation about. The thinking here is that if you are debugging an issue on an AMP-first site (Standard mode), you may want to compare the AMP page with a non-AMP page to see what is going on. Think of, for example, adding a new block that tries to enqueue some JS (like a Countdown). Nevertheless, allowing paired browsing may not have the desired effect. It could be that the theme is not even designed to work without AMP, so trying to load the theme without AMP could result in a broken template. The alternative would be to expose the debug flags (#1294, being worked on in #3448), specifically the debug option to disable AMP. Come to think of it, this would essentially be a corollary to the addition of the “Preview AMP” button (#2934 via #3323): on a Standard-mode site, the Validated URL screen should provide a “View non-AMP” link which would provide that query var to disable AMP for the request. This may have a better result than trying to force paired browsing to work in Standard mode, when there's no guarantee that non-AMP version would even work. /cc @kienstra @amedina |
050e401
to
80f0cee
Compare
The background colors are not necessary. They were used as an initial way to clearly differentiate the two. |
In 964f981, I've added a Paired Browsing navigation bar: |
I'm not sure what this means. Would it be closing the tab that was opened, or upon exiting it redirects the parent window to the URL from one of the iframes? I think closing the tab would be the better option here, but there is one major caveat with that approach. It is possible to close the tab once its opened from the "Start paired browsing" link, but not so if the page is accessed directly from that same link |
Could a reason be given for this? I can't think of any scenarios where something like this would occur. |
|
||
app.registerClientWindow( window ); | ||
|
||
document.addEventListener( 'DOMContentLoaded', () => { |
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.
Could use @wordpress/dom-ready
instead as we do already everywhere else. Example:
/**
* WordPress dependencies
*/
import domReady from '@wordpress/dom-ready';
domReady( () => {
// ...
} );
It's basically the same, but also checks document.readyState
templates/amp-paired-browsing.php
Outdated
<img src="<?php echo esc_url( amp_get_asset_url( 'images/amp-white-icon.svg' ) ); ?>" alt=""> | ||
</li> | ||
<li> | ||
<span>Paired Browsing</span> |
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.
<span>Paired Browsing</span> | |
<span><?php esc_html_e( 'Paired Browsing', 'amp' ); ?></span> |
My understanding would be the latter. If my website is the only tab open and I enter paired browsing, how do I get back to my website? Closing the tab would not be ideal.
If I disable the admin bar from being shown on the frontend (can be done in the user profile), how can I still use paired browsing? |
Is this what you had in mind for the todo: "Allow paired browsing while admin bar is not showing", @westonruter? |
You know what paired browsing will eventually be able to make excellent use of? Instead of iframes it could use portals, when they become available. This would allow seamlessly exiting the paired browsing context without having to reload the page. I think this will also work when entering paired browsing as well:
This brings something to light. A user could actually exit to either the AMP or non-AMP window when they are done with paired browsing. At the moment it forces the user to go to the non-AMP version (see https://github.com/ampproject/amp-wp/pull/3656/files#r352265423) but eventually they could be directed to either. There's no need to implement this now, but once portals become an option, this could be a nice touch. |
I have an idea for fixing the overlap problem and also exit link returning the user to where they came from: we can just turn both “Non-AMP” and “AMP” into links and keep them updated to be the same as the iframe |
That looks much neater! I love the use of the background colors in the nav bar to help differentiating the iframe labels. The use of the labels as links to exit paired browsing is also a great idea.
Yes! The use of that API would fit perfectly here and would greatly improve the UX. I guess the current implementation of using |
That's right. If portals aren't available then we'd fall back to iframes. |
…nt/3365-paired-browsing * 'develop' of github.com:ampproject/amp-wp: (53 commits) Pull the built `block-libray` package from Gutenberg SVN if it does not exists (#3847) Update dependency @babel/plugin-transform-react-jsx to v7.7.4 (#3688) Update dependency @babel/plugin-proposal-class-properties to v7… (#3687) Update dependency @babel/cli to v7.7.4 (#3685) Update dependency browserslist to v4.7.3 (#3792) Update dependency postcss to v7.0.23 (#3791) Update dependency autoprefixer to v9.7.2 (#3679) For the Gallery block, use the recommended amp-lightbox-gallery Add the -o flag to composer install for production build processes Make get_dimensions() private instead of public Remove optimize-autoloader from composer.json config, will apply this in build Also rename images to slides in Test_Carousel In Carousel, rename $images to $slides Commit Alain's suggestion to make `$images` private Update src/Component/Carousel.php Commit Alain's suggestion to make the Carousel class final Make $elements propert private again Edit DocBlocks, include correcting var tag Update the DocBlock of DOMElementList::add() Clone the DOMElementList in add(), and return the clone ...
includes/class-amp-theme-support.php
Outdated
/** | ||
* Add the `data-ampdevmode` attribute to any enqueued script that the paired browsing interface | ||
* depends on. | ||
* | ||
* @since 1.3 | ||
* | ||
* @param string $tag The link tag for the enqueued script. | ||
* @param string $handle The script's registered handle. | ||
* @return string Tag. | ||
*/ | ||
public static function filter_paired_browsing_client_script_loader_tag( $tag, $handle ) { | ||
if ( self::has_dependency( wp_scripts(), 'amp-paired-browsing-client', $handle ) ) { | ||
$tag = preg_replace( '/(?<=<script)(?=\s|>)/i', ' ' . AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, $tag ); | ||
} | ||
|
||
return $tag; | ||
} |
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.
FYI: This should be able to be largely eliminated if we implement this: google/site-kit-wp#505 (comment)
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.
A big effort and a great feature.
wp_enqueue_script( | ||
'amp-paired-browsing-client', | ||
amp_get_asset_url( '/js/amp-paired-browsing-client.js' ), | ||
$dependencies, | ||
$version, | ||
true | ||
); | ||
|
||
// Force dev mode to be enabled. This ensures that the enqueued script and its dependencies | ||
// will be present when the admin bar is not showing. | ||
add_filter( 'amp_dev_mode_enabled', '__return_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.
@pierlon I just realized that this makes every page invalid AMP, because every page is forced to be in dev mode and the custom script is enqueued. Two options I see:
- Short-circuit this function if an user is not logged-in (or rather, if
amp_is_dev_mode()
returnsfalse
). Since search crawlers will never be logged-in, they will never then see an invalid AMP page. This would also require entire paired browsing app shouldwp_die()
if the user is not logged in. - Go back to only including this JS if the admin bar is showing. Accessing paired browsing here would still need to include the
wp_die()
if! amp_is_dev_mode()
.
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 #3863.
I started experimenting with portals in #3864. |
Summary
Fixes #3365.
Todo
Add support for paired browsing while in Standard mode.Checklist