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

Create a wp-cli script to test AMP compatibility of native WordPress widgets #839

Closed
kienstra opened this Issue Jan 2, 2018 · 8 comments

Comments

Projects
5 participants
@kienstra
Copy link
Collaborator

kienstra commented Jan 2, 2018

Acceptance criteria:
AC1: Create a wp-cli script in the directory bin/.
AC2. This will add widgets to a sidebar, in order to test their AMP compatibility (see this script for reference).
AC2: Add an instance of every native WordPress widget.
AC3: The success message should simply ask the user to visit a page that has the sidebar. It doesn’t need to create a test page, like in create-embed-test-post.php.
AC4: Need to conduct a Discovery to determine all native WordPress sidebar widgets, and the support that exists for them today. Screenshot: https://d.pr/i/vBQaFt .
AC5: One story will be created as an outcome of the Discovery to address enhanced/added support for sidebar widgets.

Tasks

  • Create script, like was done for embeds, that generates core widgets in all their relevant variations, to populate the first registered sidebar it the theme.

@MackenzieHartung MackenzieHartung added this to To Do in v0.7 Jan 4, 2018

@ThierryA ThierryA added this to the v0.7 milestone Jan 8, 2018

@kopepasah

This comment has been minimized.

Copy link

kopepasah commented Jan 8, 2018

@kienstra regarding AC2, I am not sure this will always work, as sometimes there may not be a sidebar. If we can assume that there is always at least one sidebar and to use the first registered sidebar in $GLOBALS['wp_registered_sidebars'], this this method could work.

An alternative method, however, could be creating a test page (much like in the script you mentioned) and filtering the_content to only display widgets.

Any thoughts?

CC @ThierryA @MackenzieHartung

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 9, 2018

See also the Monster Widget

@kopepasah I think the script should just add the widgets to the first registered sidebar.

I think the script should skip adding a given type of widget if the given instance data already exists in the sidebar for that given type. That way we can just re-invoke the script to replenish a sidebar after some widgets have been deleted. The script should also sometimes create multiple instances of a given widget type when its instance properties vary the widget in significant ways. In particular I think of the Categories widget and how it allows you to list the categories as a dropdown instead of an unordered list. The dropdown will not be AMP-compatible because it depends on an inline script, whereas the UL will be fine. So this is the kind of thing that will be needed in the development of the script: finding the various permutations of core widgets that can result in any output that would not be AMP-compatible, and ensuring all are included in the script.

@MackenzieHartung MackenzieHartung moved this from To Do to Definition in v0.7 Jan 9, 2018

@MackenzieHartung MackenzieHartung moved this from Definition to To Do in v0.7 Jan 9, 2018

@ThierryA

This comment has been minimized.

Copy link
Collaborator

ThierryA commented Jan 10, 2018

@kopepasah could you push what you have been working on so that a BE Engineer can take over please?

@kopepasah

This comment has been minimized.

Copy link

kopepasah commented Jan 10, 2018

@ThierryA I do not have push access to neither this repo nor our internal fork.

@kopepasah

This comment has been minimized.

Copy link

kopepasah commented Jan 10, 2018

Originally I was building this script to add all necessary widgets to a sidebar (AC2), but I think it would be a good idea to create a AMP test widget (similar to the Monster Widget mentioned by @westonruter). This way the test widget can easily be programmatically added and removed as needed.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 10, 2018

@kopepasah you should have access to push now to https://github.com/xwp/amp-wp

The Monster Widget serves the same purpose as what we need, but I think it's not ideal as it nests widgets inside of each other. It also is likely designed as it is to allow a user to easily drag it into a sidebar, without the availability of the command line. However, we have WP-CLI at our disposal now, and we can leverage it to fully populate a sidebar with all the various instance variations that we want to test for.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Jan 11, 2018

In Progress

Hi @ThierryA and @westonruter,
If it's alright, I'm going to work on this.

@kienstra kienstra moved this from To Do to In Progress in v0.7 Jan 11, 2018

@ThierryA

This comment has been minimized.

Copy link
Collaborator

ThierryA commented Jan 11, 2018

Sure, go for it @kienstra

kienstra added a commit that referenced this issue Jan 13, 2018

Issue #839: wp-cli script to add test widgets to sidebar.
Adds an instance of every default WordPress widget
to the first registered sidebar.
First, it checks if a widget with the same settings exists in the sidebar.
Then, it conditionally creates the widget.
And adds all of them to the sidebar.
Mainly copies amp_get_widget_id() from import_theme_starter_content().
Props @westonruter.

kienstra added a commit that referenced this issue Jan 13, 2018

Issue #839: Fix issue in image URL, height, and width.
Use get_attachment_url() to get the image's URL.
Also, access the height and width with array index notation,
Not with object operators.
Also, pass array() as a second argument to get_option().
This will ensure that the default return is array().

@kienstra kienstra moved this from In Progress to Ready for Review in v0.7 Jan 13, 2018

ThierryA added a commit that referenced this issue Jan 15, 2018

ThierryA added a commit that referenced this issue Jan 15, 2018

ThierryA added a commit that referenced this issue Jan 15, 2018

ThierryA added a commit that referenced this issue Jan 15, 2018

ThierryA pushed a commit that referenced this issue Jan 15, 2018

@ThierryA ThierryA moved this from Ready for Review to QA in v0.7 Jan 15, 2018

@ThierryA ThierryA closed this Jan 15, 2018

@ThierryA ThierryA moved this from QA to Ready for Merging in v0.7 Jan 18, 2018

@ThierryA ThierryA added the Sprint 2 label Mar 8, 2018

@ThierryA ThierryA moved this from Ready for Merging to Beta Release in v0.7 Mar 8, 2018

@kevincoleman kevincoleman moved this from Beta Release to Production Release in v0.7 May 8, 2018

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