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

Allow showing admin bar on frontend #4546

Closed
wants to merge 13 commits into from
Closed

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Sep 18, 2020

Summary

If users prefer to display the toolbar on the frontend, they should be able to see it also when viewing stories.

Relevant Technical Choices

To-do

  • Ensure necessary scripts and styles for plugins extending the admin bar are enqueued.

User-facing changes

If enabled in the user profile, the admin toolbar will be displayed on the frontend.

Testing Instructions


Depends on #4513

@swissspidy swissspidy added the Group: WordPress Changes related to WordPress or Gutenberg integration label Sep 18, 2020
@google-cla google-cla bot added the cla: yes label Sep 18, 2020
@github-actions

This comment has been minimized.

@swissspidy swissspidy force-pushed the add/admin-bar-on-frontend branch 3 times, most recently from eefc2c8 to 3cecb71 Compare September 22, 2020 14:01
@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature AMP Output Issues related to AMP output and validation labels Sep 22, 2020
@swissspidy swissspidy force-pushed the add/amp-wp branch 3 times, most recently from cdb6b26 to da5c804 Compare September 23, 2020 21:47
@swissspidy swissspidy changed the title [WIP] Allow showing admin bar on frontend Allow showing admin bar on frontend Sep 23, 2020
@swissspidy swissspidy marked this pull request as ready for review September 23, 2020 21:56
@swissspidy
Copy link
Collaborator Author

@westonruter @schlessera @spacedmonkey While we're at it... I figured it might be nice to allow displaying the admin bar on the frontend.

@spacedmonkey
Copy link
Contributor

If we have unit tests this, I am happy with this change

@schlessera

This comment has been minimized.

@swissspidy

This comment has been minimized.

@codecov

This comment has been minimized.

Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@westonruter
Copy link
Collaborator

@westonruter
Copy link
Collaborator

Nevertheless, I don't think what I added is sufficient because it's only accounting for a one-off case for the AMP plugin. What about other plugins that add admin bar menu items? For example, Yoast is broken:

image

I assume Site Kit would also be broken.

I think it needs to do all enqueued stylesheets coming from admin_bar_init.

Something else that is needed is the admin bar needs to continue to be omitted when the story is shown in an amp-story-player. It doesn't look good now:

image

@westonruter

This comment has been minimized.

@westonruter
Copy link
Collaborator

Nevertheless, I don't think what I added is sufficient because it's only accounting for a one-off case for the AMP plugin. What about other plugins that add admin bar menu items? For example, Yoast is broken:

Unfortunately just doing wp_styles()->do_items() is not going to fix the problem for Yoast. The reason is that it is doing this:

add_action( 'wp_enqueue_scripts', [ $this, 'enqueue_assets' ] );
add_action( 'admin_enqueue_scripts', [ $this, 'enqueue_assets' ] );

It would work automatically if it instead did:

add_action( 'admin_bar_init', [ $this, 'enqueue_assets' ] );

Nevertheless, Site Kit is doing the same as Yoast:

add_action( 'admin_enqueue_scripts', $admin_bar_callback, 40 );
add_action( 'wp_enqueue_scripts', $admin_bar_callback, 40 );

Since both Site Kit and Yoast are doing the same thing, I'm not sure what alternative we have to just going ahead and doing wp_enqueue_scripts(). The challenge then is going to be omitting the stylesheets that are not intended for styling the admin bar. One way to address this could be after calling wp_enqueue_scripts() to loop over wp_styles()->queue and dequeue any stylesheet that neither a dependency of nor for admin-bar. The AMP plugin uses this to figure out what stylesheets to automatically add data-ampdevmode attributes to: https://github.com/ampproject/amp-wp/blob/806794efd3a04ce43e8f41357cf8f8692495218d/includes/class-amp-theme-support.php#L1481-L1499

Something else to keep in mind is that there are scripts that the admin bar depends on, especially plugins. Site Kit and Jetpack both will enqueue scripts that are needed or else some functionality in the admin bar that they add in PHP will either be broken or limited in functionality. See also the AMP plugin logic for marking admin bar scripts for dev mode: https://github.com/ampproject/amp-wp/blob/806794efd3a04ce43e8f41357cf8f8692495218d/includes/class-amp-theme-support.php#L1481-L1499

You may want to refer to how the admin bar is added to the legacy AMP Reader mode template. It actually goes ahead and calls wp_print_head_scripts() and wp_print_footer_scripts(): https://github.com/ampproject/amp-wp/blob/806794efd3a04ce43e8f41357cf8f8692495218d/includes/amp-post-template-functions.php#L30-L32

@swissspidy
Copy link
Collaborator Author

Thanks a lot for digging into this @westonruter! I‘m gonna try the printing scripts option

@swissspidy
Copy link
Collaborator Author

One way to address this could be after calling wp_enqueue_scripts() to loop over wp_styles()->queue and dequeue any stylesheet that neither a dependency of nor for admin-bar

Site Kit's admin bar CSS doesn't have admin-bar as its dependency, so I don't think this is viable.

@swissspidy swissspidy marked this pull request as draft September 28, 2020 20:28
@westonruter
Copy link
Collaborator

That seems like an improvement to be made with Site Kit then. Nevertheless, there's going to be many plugins that don't do this either.

It seems the scripts and styles just need to be printed in their entirety? In Jetpack we've had to manually add the data-ampdevmode attribute via the style_loader_tag filter, for example. Site Kit does this also, I believe, so it doesn't need have the dependency.

The concern is stylesheets coming from the theme and plugins which aren't related to the admin bar.

@swissspidy
Copy link
Collaborator Author

Yup, looks like it. Unrelated CSS might of course mess with the story content, so I suppose we have to make a tradeoff here.

@spacedmonkey
Copy link
Contributor

Could we make this simpler and not support plugins that add options to the admin bar? Deregister all menus items in the admin bar that are not core. Than have a filter to white list what we want. So maybe, yoast, jetpack and amp plugin can be support.

I feel like enqueueing scripts and styles that any old plugin put on the page is going complete mess.

I might be wrong here.

@swissspidy
Copy link
Collaborator Author

That'd could be a way to work around the plugin, but also requires some work (remove all menu items), and might confuse users ("where's my AMP plugin menu item?!?!")

Not sure loading all these scripts or styles is really going to be messy.

Base automatically changed from add/amp-wp to main October 14, 2020 15:10
@swissspidy swissspidy linked an issue Oct 30, 2020 that may be closed by this pull request
@swissspidy swissspidy added the Status: Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Dec 19, 2020
Base automatically changed from main to _main March 18, 2021 18:48
@swissspidy swissspidy closed this Mar 18, 2021
@swissspidy swissspidy deleted the branch main March 18, 2021 18:51
@swissspidy swissspidy reopened this Mar 18, 2021
@swissspidy swissspidy changed the base branch from _main to main March 18, 2021 19:01
@swissspidy swissspidy closed this Jul 1, 2021
@swissspidy swissspidy deleted the add/admin-bar-on-frontend branch July 1, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Output Issues related to AMP output and validation Group: WordPress Changes related to WordPress or Gutenberg integration Status: Stale Gives the original author opportunity to update before closing. Can be reopened as needed. Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend: Support showing admin bar
4 participants