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

Add compatibility with themes and plugins #123

Merged
merged 26 commits into from May 12, 2016

Conversation

Projects
None yet
2 participants
@valendesigns
Copy link
Member

commented Apr 29, 2016

Fixes #82
Fixes #103

@valendesigns valendesigns added this to the 0.6 milestone Apr 29, 2016

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

@valendesigns I think we should have a separate class for each theme/plugin adding compatibility for. Eventually the theme/plugin should copy the compatibility into their own codebases, and then we can just delete our classes.

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

@westonruter We could do it that way, sure. One thing that is not exactly clear though with partials is how to extend the JS and replace the selector for themes. Should we use an event or is there a way to override the selector in a simpler way?

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

@valendesigns That's a good question. I think that we can have a registry of the partials defined in PHP, with the selectors and render callbacks for each. And then the theme can filter this array. Or rather, it can be perhaps a theme support feature. That being the case, we can actually register the partials with PHP statically instead of dynamically with JS, or there can be a hybrid where theme partial configuration is fed into the filter for dynamic partials but the selectors and configurations (e.g. fallback_refresh, container_inclusive) get exported to JS for dynamic instantiation. (And yes, we actually do need to do dynamic instantiation to properly handle posts that get added dynamically to the document, such as for infinite scroll.)

foreach ( array( 'theme', 'plugin' ) as $type ) {
foreach ( glob( dirname( __FILE__ ) . '/' . $type . '-support/class-*.php' ) as $file_path ) {
if ( is_readable( $file_path ) ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 30, 2016

Contributor

I think the is_readable() is unnecessary?

* @access public
*/
public function add_support() {
add_filter( 'customize_posts_excluded_post_types', array( $this, 'excluded_post_types' ) );

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 30, 2016

Contributor

Instead of introducing a new filter, why not just assign it on the object itself?

add_action( 'wp_loaded', function() {
    $obj = get_post_type_object( 'feedback' );
    if ( $obj ) {
        $obj->show_in_customizer = false;
    }
} );

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 2, 2016

Author Member

We can't use closures, but this technique could work instead a using a filter.

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2016

Contributor

That's right. I just meant my code example as a proof of concept.

@@ -39,23 +40,26 @@
// Post field partial for post_title.
partial = new api.previewPosts.PostFieldPartial( id + '[post_title]', {
params: {
settings: [ id ]
settings: [ id ],
selector: api.previewPosts.data.partialSelectors.title || '.entry-title'

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2016

Contributor

Good that you added a fallback selector. But I think it has to include the baseSelector here as well.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 2, 2016

Author Member

I can move it from customize-post-field-partial.js to here.

* @param WP_Post $post Post object.
* @return string|null
*/
$rendered = apply_filters( 'customize_posts_rendered_partial', $rendered, $partial, $context, $post );

This comment has been minimized.

Copy link
@westonruter

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 2, 2016

Author Member

The only thing there would be that the filter is like customize_partial_render_{$partial->id} but id is like post[:post_type][:post_id][:field_id] and would be strange to filter on.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 2, 2016

Author Member

Never mind, I misunderstood. You were only suggesting we remove the last filter.

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2016

Contributor

That's right. customize_posts_rendered_partial is redundant and is already in core.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 2, 2016

Author Member

Should we change the other filter to be "customize_partial_render_{$partial->field_id}" to follow the same pattern wp-customize-partial.php?

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 2, 2016

Author Member

Or "customize_posts_partial_render_{$partial->field_id}"?

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2016

Contributor

Why do we need a filter here? It's not being used in this PR, and it seems the theme should be configuring these partials on the input side of things (the partial configs) rather than the output, as presumably they would have supplied render_callback args that properly generate the right markup as-is.

@westonruter westonruter changed the title [WIP] Compatibly Class Compatibly Class May 2, 2016

@westonruter westonruter changed the title Compatibly Class Add compatibility with themes and plugins May 2, 2016

*/
public function show_in_customizer() {
if ( Jetpack::is_module_active( 'contact-form' ) ) {
get_post_type_object( 'feedback' )->show_in_customizer = false;

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2016

Contributor

For good measure, I'd probably make sure that the feedback is actually registered. For example:

if ( ! Jetpack::is_module_active( 'contact-form' ) ) {
    return;
}
$post_type_object = get_post_type_object( 'feedback' );
if ( ! $post_type_object ) {
    return;
}
$post_type_object->show_in_customizer = false;

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 2, 2016

Author Member

Seems sane 👍

@@ -123,6 +135,7 @@
partial = new api.previewPosts.PostFieldPartial( id + '[post_author][avatar]', {
params: {
settings: [ id ],
selector: api.previewPosts.data.partialSelectors.author.avatar || '.vcard img.avatar',

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2016

Contributor

For each of the dynamic partials being registered, there needs to be checks to see if the partialSelectors object actually exists: api.previewPosts.data.partialSelectors.author.avatar. Consider, for example, themes that don't make use of a given template part.

$exported = array(
'isPostPreview' => is_preview(),
'isSingular' => is_singular(),
'queriedPostId' => $queried_post_id,
'settingProperties' => $setting_properties,
'partialSelectors' => $partial_selectors,

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2016

Contributor

Instead of exporting just the selectors, how about exporting the entire configs for the partials? For example, it could export here the container_inclusive, fallback_refresh, and other properties that will be used when the partials are registered on the client. This data can also then be extracted from being calculated in this method to instead be in a dedicated method, and it can then be used in \WP_Customize_Posts_Preview::filter_customize_dynamic_partial_args() instead of hard-coding those in WP_Customize_Post_Field_Partial:

if ( ! empty( $args['placement'] ) ) {
if ( ! isset( $args['container_inclusive'] ) ) {
$args['container_inclusive'] = true;
}
if ( ! isset( $args['fallback_refresh'] ) ) {
$args['fallback_refresh'] = false;
}
}

@westonruter

This comment has been minimized.

Copy link
Contributor

commented May 2, 2016

@valendesigns one more thought: instead of having a default list of the post field partials that a theme is expected to support and the selectors for each, should these configurations not actually be added to Customize_Posts_Theme_Support subclasses for each Core theme?

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented May 2, 2016

@weston That is a thought I had, but wanted to get some more input from you before going much further. with this PR. Essentially we need to come up with the best way to pass the partial config to the JS so it can be be added dynamically. You touched on that in one of the comments above, though that means we are relying on the support class to tell us which partials are supported. Which makes complete sense, we just need a good reusable schema to make this work well.

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented May 2, 2016

@westonruter So we need to let the support class dictate the config, which is exported to JS and used to set the $args in WP_Customize_Post_Field_Partial:__constructor. The question then is how should the support class override the config?

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented May 2, 2016

Also if the theme has not set a config then does that mean we should not register any partials?

@westonruter

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

@valendesigns

So we need to let the support class dictate the config, which is exported to JS and used to set the $args in WP_Customize_Post_Field_Partial:__constructor. The question then is how should the support class override the config?

There can be a method like WP_Customize_Posts::get_post_field_partial_configs() which can have a filtered response. The theme supports can add filters for this response.

Also if the theme has not set a config then does that mean we should not register any partials?

If the theme doesn't have a config set for partials, then we can just register the defaults which are supported by 99% of themes. For example any partials that target the_content() via an .entry-content selector: anything that makes use of the Microformat classes or anything output by post_class() should be safe to use as defaults.

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

@westonruter I added the defaults which themes can unregister via the filter or modify as they see fit. I feel the defaults are fairly standard across many of the Twenty Something themes, there will be minor tweaks I'm sure, but overall themes should be using these partials. Have you seen the latest commit?

'container_inclusive' => true,
),
'post_author' => array(
'biography' => array(

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

Do you think biography is standard enough to be a default?

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 3, 2016

Author Member

I suppose bio is not going to be in all themes, we can remove it from the default list.

'comments-area' => array(
'selector' => '.comments-area',
'body_selector' => true,
'singular_only' => true,

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

What are body_selector and singular_only? I don't see them referenced anywhere.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 3, 2016

Author Member

body_selector tells the JS to link the partials base selector to the body instead of the article, singular_only is to only load the partial on single posts, archive_only loads the partial everywhere but the single post. We can rework these, but they are needed to make sense of the context of a partial.

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

I see. I was looking for body_selector not bodySelector as it is named in JS.

),
'comments-link' => array(
'selector' => '.comments-link',
'archive_only' => true,

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

Also, what is archive_only?

* @param array $schema Partial schema.
* @return array
*/
$schema = apply_filters( 'customize_posts_partial_schema', $schema );

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

👍

'selector' => '.vcard img.avatar',
'container_inclusive' => true,
'fallback_refresh' => false,
),

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

I liked the flatter schema format you had with [post_author][avatar] as keys, although I suppose the brackets are unsightly. Was that not used because of the trouble parsing out the parts? What if a theme wants to use a placement called selector? This would break the previewPosts.partialSchema logic for detecting nested structures.

What about using a key structure like post_author[avatar]? This would be easy to parse, as it's just removing all ] and splitting on [.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 3, 2016

Author Member

We could check that selector is typeof string which would solve that?

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

True. But is it better than having a flatter structure?

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 3, 2016

Author Member

A flatter structure is nice sure, but then you couldn't do this in the filter...

$schema['post_author']['biography'] = array(
    'selector' => '.author-info',
    'singular_only' => true,
    'container_inclusive' => true,
);

You would be doing this instead...

$schema['[post_author][biography]'] = array(
    'selector' => '.author-info',
    'singular_only' => true,
    'container_inclusive' => true,
);

Which in my experience I believe will only confuse the masses.

This comment has been minimized.

Copy link
@westonruter

westonruter May 3, 2016

Contributor

Or:

$schema['post_author[biography]'] = array(
    'selector' => '.author-info',
    'singular_only' => true,
    'container_inclusive' => true,
);

Having multidimensional IDs are already very commonplace in the Customizer, e.g. widget_text[123] and colors[background]. So to me it seems having a multidimensional ID for the partials schema fits nicely.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 3, 2016

Author Member

I'll change it then 👍

add_customize_posts_support( 'Customize_Posts_Twenty_Ten_Support' );
}
add_action( 'after_setup_theme', 'twentyten_support' );

This comment has been minimized.

Copy link
@westonruter

westonruter May 10, 2016

Contributor

I think this might be a better approach:

add_action( 'customize_register', function ( $wp_customize ) {
    if ( isset( $wp_customize->posts ) ) {
        $wp_customize->posts->add_support( 'Customize_Posts_Twenty_Ten_Support' );
    }
});

or

add_action( 'customize_register', function ( $wp_customize ) {
    if ( isset( $wp_customize->posts ) ) {
        $wp_customize->posts->add_support( new Customize_Posts_Twenty_Ten_Support( $wp_customize->posts ) );
    }
});
$wp_customize_posts_support[ $class_name ] = $support;
}
}
}

This comment has been minimized.

Copy link
@westonruter

westonruter May 10, 2016

Contributor

I think we can have $wp_customize->posts->add_support() which adds the instance to $wp_customize->posts->supports instead of a global $wp_customize_posts_support.

@westonruter westonruter merged commit 701058c into develop May 12, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 97.646%
Details

@westonruter westonruter deleted the feature/compat branch May 12, 2016

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.