wp-shortcake / shortcake

Shortcake makes using WordPress shortcodes a piece of cake.
GNU General Public License v2.0
665 stars 143 forks source link

Deactivate method on Media Controller? #785

Closed elliott-stocks closed 6 years ago

elliott-stocks commented 6 years ago

I noticed this todo on the select2 field. But as far as I can tell, the destroying on select2 doesn't actually happen;

/**
 * Extending SUI Media Controller to hide Select2 UI Drop-Down when menu
 * changes in Meida modal
 * 1. going back/forth between different shortcakes (refresh)
 * 2. changing the menu in left column (deactivate)
 * 3. @TODO closing the modal.
 */

I need to be able to detect when the modal closes, to trigger an action.

This is the code I used -

var mediaController = sui.controllers.MediaController;
sui.controllers.MediaController = mediaController.extend({
    deactivate: function() {
        console.log( 'do something');
    }
});

However this doesn't work, am I possibly calling it to early?

I'm thinking would it not be easier to add deactivate method on the media controller, then trigger a hook?

On MediaController initialize -

this.on( 'deactivate', this.deactivate, this );

Then the function that triggers a hook -

deactivate: function() {
    var hookName = 'shortcode-ui.frame_deactivate';
    wp.shortcake.hooks.doAction( hookName, this );
}

I'm happy to open a PR for this, if you think it'd make sense? Would then resolve the select2 UI issue too.

goldenapples commented 6 years ago

Does the render_destroy hook do what you want? It looks like it's not currently being called if the media modal is closed without inserting to the editor, but it probably should be.

elliott-stocks commented 6 years ago

Thanks, @goldenapples. I'm using that already, but as you mentioned it isn't called on modal close.

Adding this hook to the deactivate method seems like it would work, thoughts?

goldenapples commented 6 years ago

Adding this hook to the deactivate method seems like it would work, thoughts?

Seems feasible. However, since we're overriding the core media controller, I'd be wary of overloading it too much. What do you think about attaching it in the one-time "close" callback here, so that the hook only gets called in the shortcode UI frame and not in every media controller?

desaiuditd commented 6 years ago

~I would keep the same shortcode-ui.render_destroy hook in the one-time "close" callback, rather than introducing a separate hook. Thoughts?~

Never mind. Found my answer in #797

goldenapples commented 6 years ago

Fixed in #797 - thanks!