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

Issue #839: Add wp-cli script to create test widgets #859

Merged
merged 7 commits into from Jan 15, 2018

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Jan 13, 2018

Request For Code Review

Hi @westonruter or @ThierryA,
Could you please review this pull request for #839? This builds on @kopepasah's work to create a widget test script.

Using Koop's function, it finds the first registered sidebar. Then, it adds an instance of every WordPress widget that's registered in a "vanilla" instance, per Joshua's screenshot.

But only if an instance with the same settings doesn't already exist in that sidebar, As @westonruter requested in #839.

Some widgets have multiple instances, with different settings. Like 'Audio,' 'Archives,'Categories,' 'Tag Cloud,' and 'Video.'

kopepasah and others added 3 commits January 10, 2018 13:52
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.
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().
Copy link
Collaborator

@ThierryA ThierryA left a comment

Choose a reason for hiding this comment

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

Great work @kienstra 👍 I pushed a quick fix and added documentation for the script, could you kindly review my changes.

$menus = wp_get_nav_menus();
$minimum_count = 4;
foreach ( $menus as $menu ) {
if ( $menu->count > $minimum_count ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually requires 5 menu items.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed here.

*/
function amp_widget_already_in_sidebar( $widget, $sidebar ) {
$sidebars = wp_get_sidebars_widgets();
$widgets_in_sidebar = $sidebars[ $sidebar ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure this isset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed here.

contributing.md Outdated
1. `ssh` into an environment like [VVV](https://github.com/Varying-Vagrant-Vagrants/VVV)
2. `cd` to the root of this plugin
3. run `wp eval-file bin/add-test-widgets-to-sidebar.php`
4. visit a page with the sidebar: Blog Sidebar.
Copy link
Contributor Author

@kienstra kienstra Jan 15, 2018

Choose a reason for hiding this comment

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

Hi @ThierryA,
Thanks for adding this documentation. This step 4. will often be accurate. But the step below would apply no matter what theme is active:

4. There will be a message indicating which sidebar has the widgets. Please visit a page with that sidebar.

@kienstra
Copy link
Contributor Author

Thanks For Reviewing And Improving
Made Minor Point

Hi @ThierryA,
Thanks a lot for reviewing this and adding documentation. I only added a minor point above about the documentation, but it's not a blocker to merging. If you'd like, I'd be happy to apply the suggestion to this branch.

@ThierryA
Copy link
Collaborator

Thanks @kienstra, I pushed a commit with your suggestion 👍

@ThierryA ThierryA merged commit 75ed4a9 into develop Jan 15, 2018
@ThierryA ThierryA deleted the add/839-wpcli-script-for-widgets branch January 15, 2018 21:46
@westonruter westonruter added this to the v0.7 milestone Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants