wp-shortcake / shortcake-bakery

A fine selection of shortcodes for WordPress
42 stars 16 forks source link

Add filter to disable embed feature #196

Closed davisshaver closed 7 years ago

davisshaver commented 7 years ago

Adds filter to Shortcake Bakery that lets a user disable the Embed feature with a filter on the admin JS and and button. (Also cleaned up for PHPCS.) Splits up admin and shortcake bakery shortcode support JS to support this.

    add_filter( 'shortcake_bakery_embeds', array( $this, 'filter_shortcake_bakery' ) );

    /**
     * Disable the embed feature in Shortcake bakery
     *
     */
    function filter_shortcake_bakery( $shortcake_bakery_embeds ) {
        return false;
    }
davisshaver commented 7 years ago

@goldenapples Any thoughts on merging this?

goldenapples commented 7 years ago

I think this is very reasonable; there are plenty of cases where you'd want to only expose the "Add embed" buttons for certain users etc.

I'm not very clear on the naming of the script handles and the filter - I'm thinking that maybe renaming the shortcake-bakery-admin.js script to something like shortcake-bakery-add-embed-media-frame.js might make that functionality clearer. And giving the filter name a verb, like shortcake_bakery_show_add_embed would make it more clear to me.

One thing that needs a bit of tweaking is that the js variable ShortcakeBakery.shortcodes is used in shortcake-bakery-shortcodes.js, so that will have to be localized separately from the other vars on ShortcakeBakery, or else it won't be available if the add embed script is disabled by filter.

davisshaver commented 7 years ago

@goldenapples thanks for the review! Cleaned up based on the feedback and seems good locally.

goldenapples commented 7 years ago

Looks good to me!

The only thing I'm trying to figure out is where this build thrashing is coming from. I've tried reinstalling my entire node_modules directory and re-running grunt, and I'm still getting a different output from you here:

screen shot 2016-12-19 at 8 59 06 am

Can you just try reinstalling your node modules and running grunt and see if you still get the same result?

(This is trivial and not related to this PR, just a style issue that I'd like to figure out...)

davisshaver commented 7 years ago

@goldenapples I'm using node 7, maybe that's source of difference? Tried nuking node_modules and re-installing, but no diff.

davisshaver commented 7 years ago

@goldenapples Let me know what makes sense, I can try from a different computer/node version in a bit.

goldenapples commented 7 years ago

Oh, OK. That probably explains it. I hadn't updated from node 0.12.x yet. I just tried installing v7, but now I'm in a mess of broken dependencies where node-sass won't seem to install at all. :( I'll dig through this and figure out what's going on with my build. In the meantime, I don't see any harm in merging this. It'd be good to nail down the dev dependencies to avoid the build thrashing before a new release.

goldenapples commented 7 years ago

OK, I really have no idea here. Updated to Node v7.0.0 and npm v4.1.1. Had to update grunt-sass and node-sass in the modules here in order to run them. But I'm still getting different output in the browserified js file headers. Like I said, it's not really an issue with this PR, so I think the best approach is to merge this and then do some cleaning up in a separate PR that updates all of the dev dependencies to current versions. I'll do that and then tag you for review.

davisshaver commented 7 years ago

Thanks Than! 🌮