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

Revamp widget connector #391

Merged
merged 48 commits into from Jul 10, 2014

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Contributor

commented Apr 6, 2014

Resolves (in part) #308

This adds support for tracking widget changes in the customizer, improves the logic for tracking changes on the widgets admin page, and allows tracking of widget changes no matter how they are made (e.g. via WP-CLI).

New features:

  • Log changes to widgets in the customizer
  • Handle batch operations on widgets
  • Add deactivated/reactivated actions
  • Add basic support for old single widgets
  • Remove hooks which tie into the UI, and rely solely on actions which fire when the underlying data is modified.
  • Note: sidebar Stream meta has been renamed to sidebar_id to correspond with widget_id. No migration script to rename stream to stream_id has been included.

Todo:

  • Improve behavior of logging movement of widgets between sidebars when in the customizer.
  • Should widget_id ever be added as a context?
  • When moving a widget from one sidebar to another, should there be one Stream record with two contexts, or two records with one context each?
  • Add database migrations for any changed stream meta (e.g. sidebar => sidebar_id)
  • Add widget edit link, in addition to the widget area edit link. (See #521)

westonruter added some commits Mar 29, 2014

Eliminate reference to undefined variable
Argument was passed to wp_stream_post_insert_error action
Track widget movement to other sidebars
Still have work to do to make this work properly in Customizer
Use a different string template mechanism which prevents args from being needlessly added to Stream meta
Prevent widget movement record if dealing with active/inactive widgets
Fix label for movement to reverse from/to sidebars
Use new tpl vars for summaries
Merge branch 'master' into widget-customizer
Conflicts:
	connectors/widgets.php
} else {
// Neither a name nor a title are available, so use the sidebar ID
$message = __( '{widget_id} widget updated', 'stream' );
}

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 6, 2014

Author Contributor

@fjarrett this may have undone what you did in 8818976

This comment has been minimized.

Copy link
@fjarrett

fjarrett Apr 7, 2014

Contributor

@westonruter Yeah it would be ideal to put the sidebar name in these summaries again, also we should use _x() to give context to the translator.

Can you help me understand the differences between $widget_id_base, $widget_id and $name? I'm a little confused.

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 7, 2014

Author Contributor

The $name is the kind of widget that it is, e.g. Text, RSS, Categories, etc. Ideally you'd want to see both the name and the title in the stream.

If the $widget_id is text-123 then the $widget_id_base is text, so just the ID without the number attached to it.

// Likely a single widget since no name is available
$message = __( '"{title}" widget updated', 'stream' );
} else {
// Neither a name nor a title are available, so use the sidebar ID

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 7, 2014

Author Contributor

Should say "widget ID"

westonruter added some commits Apr 17, 2014

Merge branch 'develop' into widget-customizer
Conflicts:
	connectors/widgets.php
Allow PHPCS and WPCS to pull from different locations
 * Point to expirimental PHPCS and WPCS for rulset subsets
 * Introduce config file for managing Travis before_script environment
 * Remove phpcs.ruleset.xml since we're referencing a core ruleset

See xwp/wp-dev-lib#30
Merge branch 'phpcs-improvements' into widget-customizer
Conflicts:
	bin/.travis.yml
	includes/admin.php
	phpcs.ruleset.xml

fjarrett added some commits Jun 11, 2014

Merge branch 'develop' into widget-customizer
Conflicts:
	connectors/widgets.php

@fjarrett fjarrett added do not merge and removed enhancement labels Jun 11, 2014

@fjarrett

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2014

Closing this PR until more progress can be made in the near future.

@fjarrett fjarrett closed this Jul 8, 2014

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2014

What was outstanding?

@fjarrett

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2014

global $wp_registered_sidebars;
if ( array_key_exists( $sidebar, $wp_registered_sidebars ) ) {
$links[ __( 'Edit Widget Area', 'stream' ) ] = admin_url( 'widgets.php#' . $sidebar ); // xss ok (@todo fix WPCS rule)
}
// @todo Also old_sidebar_id and new_sidebar_id
// @todo Add Edit Widget link

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 8, 2014

Author Contributor

These are optional nice to haves. Future enhancements.

This comment has been minimized.

Copy link
@fjarrett

fjarrett Jul 10, 2014

Contributor

So then I guess #521 should be reopened as well.

static protected function handle_widget_removal( $old, $new ) {
$all_old_widget_ids = array_unique( call_user_func_array( 'array_merge', $old ) );
$all_new_widget_ids = array_unique( call_user_func_array( 'array_merge', $new ) );
// @todo In the customizer, moving widgets to other sidebars is problematic because each sidebar is registered as a separate setting; so we need to make sure that all $_POST['customized'] are applied?

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 8, 2014

Author Contributor

I believe this was addressed in 297ccf0.

),
compact( 'name', 'sidebar_name', 'title', 'id_base', 'sidebar', 'widget_id', 'new_instance', 'old_instance' ),
$message,
compact( 'widget_id', 'sidebar_id' ), // @todo Do we care about sidebar_id in meta if it is already context? But there is no 'context' for what the context signifies

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 8, 2014

Author Contributor

An open question.

This comment has been minimized.

Copy link
@shadyvb

shadyvb Jul 20, 2014

Contributor

@westonruter context is always used in the context of the connector, so you don't really know what a context mean till you see how the connector stores/uses it. Adding it to meta shouldn't be a problem, it should even be encouraged, so it is more visible/clear in queries that might use it, instead of using the context field which might be ambiguous in queries.
/cc @fjarrett

@fjarrett fjarrett reopened this Jul 10, 2014

@fjarrett fjarrett removed the do not merge label Jul 10, 2014

fjarrett added a commit that referenced this pull request Jul 10, 2014

@fjarrett fjarrett merged commit 4476e22 into develop Jul 10, 2014

1 check passed

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

@fjarrett fjarrett deleted the widget-customizer branch Jul 10, 2014

@fjarrett

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2014

/five @westonruter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.