themeum / kirki

Extending the customizer
https://kirki.org
MIT License
1.26k stars 326 forks source link

active_callback doesn't work with transport "postMessage"? #1118

Closed kmob2 closed 7 years ago

kmob2 commented 8 years ago

Issue description:

It seems that when using active_callback along with transport "postMessage", nothing seems to happen. Is this on purpose?

I am using this exact demo configuration along with 'transport' => 'postMessage', https://aristath.github.io/kirki/docs/arguments/active_callback.html

Version used:

Latest

Using theme_mods or options?

theme_mods

aristath commented 8 years ago

Can you please post the exact code you're using? Both for the dependency control and the dependent control

aristath commented 8 years ago

@kmob2 Can you please try using the development branch from this repo and let me know if it works for you? If it doesn't then please paste the exact code you're using for both fields.

kmob2 commented 8 years ago

Hello,

I just downloaded the latest dev branch and it still doesn't seem to be working.

I am using your demo code from https://aristath.github.io/kirki/docs/arguments/active_callback.html along with 'transport' => 'postMessage',

Here is the full code

Kirki::add_config( 'my_config' );

Kirki::add_section( 'my_section', array(
    'title'          => __( 'My Section' ),
    'priority'       => 10,
) );

Kirki::add_field( 'my_config', array(
    'type'      => 'checkbox',
    'settings'  => 'my_checkbox',
    'label'     => __( 'My Checkbox', 'my_textdomain' ),
    'section'   => 'my_section',
    'transport'   => 'postMessage',
    'default'   => 0,
    'priority'  => 10,
) );

Kirki::add_field( 'my_config', array(
    'type'      => 'radio',
    'settings'  => 'my_radio',
    'label'     => __( 'My Radio', 'my_textdomain' ),
    'section'   => 'my_section',
    'default'   => 'option-1',
    'transport'   => 'postMessage',
    'priority'  => 20,
    'choices'   => array(
        'option-1' => __( 'Option 1', 'my_textdomain' ),
        'option-2' => __( 'Option 2', 'my_textdomain' ),
        'option-3' => __( 'Option 3', 'my_textdomain' )
    )
) );

Kirki::add_field( 'my_config', array(
    'type'      => 'text',
    'settings'  => 'my_setting',
    'label'     => __( 'Text Color', 'my_textdomain' ),
    'section'   => 'my_section',
    'default'   => __( 'my default text here', 'my_textdomain' ),
    'priority'  => 30,
    'transport'   => 'postMessage',
    'active_callback'  => array(
        array(
            'setting'  => 'my_checkbox',
            'operator' => '==',
            'value'    => 1,
        ),
        array(
            'setting'  => 'my_radio',
            'operator' => '!=',
            'value'    => 'option-1',
        ),
    )
) );

I am not sure on what code you are using to achieve this solution, however Wester Ruter, who works on the core development at WP for the customizer, recently posted a great code snipped to achieve this function with pure JavaScript.

https://make.xwp.co/2016/07/24/dependently-contextual-customizer-controls/

pingram3541 commented 7 years ago

This also occurs when the field your're checking against has transport postMessage. For example the following set of fields toggles whether the site is full-width or fixed width.

  1. Add/remove class "fixed" to body element
  2. Customizer Setting Callback - Reveals site width and site background-color options - not working!
  3. Customizer Preview Callback - Switches body bg color and container width
Kirki::add_field( 'mytheme', array(
    'type'        => 'radio-image',
    'settings'    => 'site_layout',
    'label'       => esc_attr__( 'Site Layout', 'mytheme' ),
    'section'     => 'site_settings',
    'default'     => 'full',
    'priority'    => 10,
    'choices'     => array(
        'full'   => plugins_url( '/kirki/assets/images/1c.png' ),
        'fixed' => plugins_url( '/kirki/assets/images/3cm.png' ),
    ),
    'transport' => 'postMessage',
    'js_vars'   => array(
        array(
            'function' => 'mytheme_site_layout',
        ),
    ),
) );

Kirki::add_field( 'mytheme', array(
    'type'        => 'color',
    'settings'    => 'site_bg_color',
    'label'       => esc_attr__( 'Site Background Color', 'mytheme' ),
    'section'     => 'site_settings',
    'default'     => '#efefef',
    'priority'    => 10,
    'alpha'       => true,
    'active_callback'    => array(
        array(
            'setting'  => 'site_layout',
            'operator' => '==',
            'value'    => 'fixed',
        ),
    ),
) );

Kirki::add_field( 'mytheme', array(
    'type'        => 'number',
    'settings'    => 'site_width',
    'label'       => esc_attr__( 'Layout Width', 'mytheme' ),
    'section'     => 'site_settings',
    'default'     => 1600,
    'choices'     => array(
        'min'  => '768',
        'max'  => 99999,
        'step' => 1,
    ),
    'active_callback'    => array(
        array(
            'setting'  => 'site_layout',
            'operator' => '==',
            'value'    => 'fixed',
        ),
    ),
) );

Also, I found that if you enqueue a script with the action ''customize_preview_init" is still fires a callback for each field regardless of whether transport is set to postMessage or js_vars is set but the value is not passed. For example, same as above, except we are using default refresh method instead of postMessage:

Kirki::add_field( 'mytheme', array(
    'type'        => 'radio-image',
    'settings'    => 'site_layout',
    'label'       => esc_attr__( 'Site Layout', 'mytheme' ),
    'section'     => 'site_settings',
    'default'     => 'full',
    'priority'    => 10,
    'choices'     => array(
        'full'   => plugins_url( '/kirki/assets/images/1c.png' ),
        'fixed' => plugins_url( '/kirki/assets/images/3cm.png' ),
    ),
) );

function mytheme_customize_preview_js() {
    wp_enqueue_script( 'mytheme_customizer', get_template_directory_uri() . '/js/customizer.js', array( 'customize-preview' ), $ver, true );
}
add_action( 'customize_preview_init', 'mytheme_customize_preview_js' );

and inside customizer.js

( function( $ ) {

    //site layout
    wp.customize( 'site_layout', function( value ){
       console.log( 'callback fires!' );
        value.bind( function( to ) {
            console.log( 'value is: ' + to );
            $( 'body' ).addClass( to );
        } );
    } );

} )( jQuery );

In the console, we see "callback fires!" on init but not every time we change the site layout field and a value for the field is never passed. So while it's a cool discovery that we can still execute a js callback for any field upon page load regardless of the transport setting, not being triggered on value changes and lacking a value passed to js results in a pretty significant handicap.

pingram3541 commented 7 years ago

Same issue - https://github.com/aristath/kirki/issues/1031

daviedR commented 7 years ago

Same issue here. The active_callback doesn't work if the dependent field has transport => 'postMessage'.

pingram3541 commented 7 years ago

I just confirmed that active_callback doesn't work if either is true. Basically postMessage kills active_callback when the setting itself and/or the setting we are checking against has transport set to postMessage.

This is by far one of the biggest limitations I am running into with building out my panels with Kirki.

aristath commented 7 years ago

Hey guys Started working on a JS implementation for this one but it's gonna take me some time... time is a bit limited atm.

aristath commented 7 years ago

I'm a bit troubled by this one... The fact that active_callback doesn't work with postMessage is not a Kirki limitation, it's a WP-Core limitation (see Weston's comment on http://wordpress.stackexchange.com/questions/211779/customiser-active-callback-not-working-on-control-with-postmessage-transport#comment307740_211779)

Implementing via JS would allow us to fix this in cases where the visibility of a control depends on the values of other controls. But this is not always the case... The beauty of active_callback is that it can be any function that returns true/false. Implementing via JS would ruin any callbacks that are not regarding other control values. I found a way around that so that it uses a JS implementation only for controls that depend on values of other controls. But that's not the case always either! There are cases where we may want mixed results... So for example show control X when value of control Y === true, AND function Z returns true.

I'm inclined to leave this as-is since as I mentioned earlier it's a WP-Core quirk (for good reason).

My fear is that fixing this will introduce more issues than it will solve.

kmob2 commented 7 years ago

In one of my first comments in this thread, I linked to Weston's blog where he has a tutorial about this. https://make.xwp.co/2016/07/24/dependently-contextual-customizer-controls/ I assume it's the same code you linked to on stackexchange from Weston.

I've been using that code (slightly modified for my own needs), and placed the code inside of the customizer.js file, and it works, no issues for over 2 months using Kirki and I have quiet a complex control panel where multiple values are nested and need to be true/false/custom values for others to become active. Didn't have any real issues. However I was always hoping that this would become part of Kirki, since my javascript skills are not that great and my personal javascript code for this, is quiet ugly to be honest.

Knowing that many themes option panel will mix and match PostMessage with Refresh, it can be project killer for developers and avoid Kirki all together.

aristath commented 7 years ago

In one of my first comments in this thread, I linked to Weston's blog where he has a tutorial about this. https://make.xwp.co/2016/07/24/dependently-contextual-customizer-controls/ I assume it's the same code you linked to on stackexchange from Weston.

Similar, yes.

@kmob2 as I mentioned in my previous comment, field dependencies is just one of the uses of the active_callback argument. On a theme I was building recently for example I had some settings that were specific to the homepage, other settings that only applied to single posts and so on. So on some settings I had this:

'active_callback' => 'is_front_page'

On another setting that was for a product I had this:

'active_callback' => function_exists( 'is_product' ) ? 'is_product' : '__return_false',

and others with similar rules.

If we implement what you're suggesting, we're automatically excluding all these use-cases that rely on PHP.

No, I'm afraid that if you take the above into account you'll realize that we can't make it rely exclusively on JS.

Knowing that many themes option panel will mix and match PostMessage with Refresh, it can be project killer for developers and avoid Kirki all together.

This is not a limitation of Kirki, it's a limitation of the customizer. Kirki simply enhances and simplifies some things in the customizer. But we can't bypass it. If developers are willing to avoid Kirki because of that, then they should avoid the customizer altogether - since that's where this issue lies.

If we implement something it will have to be well-thought and will have to work both for PHP functions like the ones I mentioned in the examples above, as well as field dependencies.

The simplest solution would be to force-deactivate postMessage in case an active_callback is used. But that would cause another wave of frustration from users/developers, wouldn't it?

So... I'm left with no solutions for now. The best thing you can do for the time being until this gets resolved somehow in WordPress Core is to write your own JS like you already do.

I will continue to investigate this and try to think of a way to handle all scenarios properly, but it's not going to happen in a day.

pingram3541 commented 7 years ago

Yes indeed, we still need active_callback to evaluate php of course. But I think we can take advantage of knowing when active_callback contains any number of arrays that match having the "setting", "operator" and "value" keys. This makes it dumb simple what to replicate in js for the same conditionals doesn't it?

I had just assumed (my bad) when using transport postMessage and active_callback we already did this and I should have looked closer so as not to have false expectation so for that I apologize.

Looking deeper, I think the part we are missing from WP core is the active_callback object being present in js so we can look for the conditional logic we need? I'm going to try an idea and report back. I think the trick I use to get post_meta data into the customizer might also work for this too by binding the active_callback object data to the wp.customize object and then do whatever we want in js once it's passed.

Otherwise it should be made clear to everyone in the docs that using some other function for active_callback would break this js enhancement.

Also maybe we can provide an example in the docs on how to pass the needed dataset to js when active_callback is also a custom function. Most folks will only need a simple nudge in the right direction on how to bind that extra data to the wp.customize object then you can use the customizer for anything want.

aristath commented 7 years ago

@pingram3541 take a look at https://github.com/aristath/kirki/tree/develop/modules/field-dependencies

The PHP is mostly done, the JS though is a mess if you've got the time to take a look at it 👍

aristath commented 7 years ago

Seems to be working in 3.0