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

Restore admin bar on AMP pages and improve AMP menu items #1219

Merged
merged 5 commits into from Jun 20, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Member

westonruter commented Jun 20, 2018

  • Show admin bar on AMP pages by default. Use a forked version of admin-bar.css from core to utilize :focus-within instead of relying on jQuery to toggle hover classes.
  • Add setting for turning off admin bar in AMP responses, since the admin bar may cause the total CSS to exceed 50KB, in particular Twenty Fifteen.
  • Improve AMP admin bar menu item:
    • Add link between AMP and non-AMP versions when on non-native.
    • Show actual validation errors when viewing admin bar in AMP.
    • Show warning emoji when there are sanitized unaccepted validation errors.
  • Add missing recognition for media attribute on style elements.
  • Fix handling of !important transformations by inserting higher-specificity rules adjacent to their original rule instead of appending to the very end of the stylesheet.
  • Automatically tree-shake obsolete IE6/IE7 CSS selector hacks.

New admin setting:

image

Link to AMP version from non-AMP page in paired mode:

image

Admin bar item that appears after having been redirected from AMP to non-AMP version due to non-sanitized validation errors:

image

Admin bar item that appears on AMP page when there is an unaccepted validation error that has been forcibly sanitized:

image

Admin bar item in native mode when there are unaccepted validation errors:

image

Fixes #1205

westonruter added some commits Jun 19, 2018

Show admin bar in AMP
* Add forked version of WP 4.9.6's admin-bar.css with fixes for AMP.
* Fix positioning of admin bar with sticky nav in Twenty Seventeen.

@westonruter westonruter added this to the v1.0 milestone Jun 20, 2018

@westonruter westonruter requested a review from kienstra Jun 20, 2018

@kienstra

Approved

Hi @westonruter,
It's great to see the admin bar on AMP URLs! This is a big step in usability.

admin-bar-url

$i = array_search( $ruleset, $css_list->getContents(), true );
if ( false !== $i ) {
$css_list->splice( $i + 1, 0, array( $important_ruleset ) );

This comment has been minimized.

@kienstra

kienstra Jun 20, 2018

Collaborator

There was originally an error from this line, but running composer install took care of it:

( ! ) Error: Call to undefined method Sabberworm\CSS\CSSList\Document::splice() in /srv/www/loc/public_html/wp-content/plugins/amp/includes/sanitizers/class-amp-style-sanitizer.php on line 1622

This comment has been minimized.

@westonruter

westonruter Jun 20, 2018

Member

Yes, that is expected 👍

</label>
</p>
<p class="description">
<?php esc_html_e( 'An additional stylesheet is required to properly render the admin bar. If the additional stylesheet causes the total CSS to surpass 50KB then the admin bar should be disabled to prevent a validation error or an unstyled admin bar in AMP responses .', 'amp' ); ?>

This comment has been minimized.

@kienstra

kienstra Jun 20, 2018

Collaborator

This isn't a blocker. But it looks like there's an extra space after 'responses':

in AMP responses .', 'amp' );

<p>
<label for="disable_admin_bar">
<input id="disable_admin_bar" type="checkbox" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[disable_admin_bar]' ); ?>" <?php checked( AMP_Options_Manager::get_option( 'disable_admin_bar' ) ); ?>>

This comment has been minimized.

@kienstra

kienstra Jun 20, 2018

Collaborator

It's great to have the admin bar enabled by default, with this checkbox to disable it.

Admins might be willing to ignore the AMP validation error of CSS over 50KB in order to have the admin bar. Of course, that error wouldn't even appear for non logged-in users

Improve AMP admin bar menu item
* Add setting for turning off admin bar
* Link between AMP and non-AMP versions when on non-native.
* Show actual validation errors when viewing admin bar AMP.
* Show warning emoji when there are sanitized unaccepted validation errors.

@westonruter westonruter force-pushed the admin-bar branch from 1f7f860 to 80ebbcb Jun 20, 2018

@postphotos

This comment has been minimized.

Contributor

postphotos commented Jun 20, 2018

This is great, @westonruter! Further brings parity to the AMP experience: My preferred way to edit a post is, on the front end as a logged-in user, to find the page and use the wp-admin bar and click Edit Post. That's now possible again, with a bonus validation feature. 👍

@westonruter westonruter merged commit 7757995 into develop Jun 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the admin-bar branch Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment