Skip to content

Conversation

@goldenapples
Copy link
Contributor

This adds a basic method for registering a callback function on an
attribute field when registering UI for a shortcode. If the attribute
callback is defined on a field and maps to the name of a valid
javascript function, that function will be called on rendering the
shortcode UI and on any changes to that attribute field.

The value of this in the callback function will be the array of view
models representing all of the attribute input field views, and the
function receives two arguments: the changed value, and the collection
of model attributes.

See #316 and #333.

An example of a callback function that uses this API is in wp-shortcake/image-shortcake#36

This adds a basic method for registering a callback function on an
attribute field when registering UI for a shortcode. If the attribute
`callback` is defined on a field and maps to the name of a valid
javascript function, that function will be called on rendering the
shortcode UI and on any changes to that attribute field.

The value of *this* in the callback function will tbe the array of view
models representing all of the attribute input field views, and the
function receives two arguments: the changed value, and the collection
of model attributes.
@goldenapples
Copy link
Contributor Author

I would love to get people's feedback on this as a potential solution to the problem of dynamically updating or showing/hiding fields based on the value of another field. I think the function signature of the callback function probably needs some thought as to possible use cases - what information would make writing callback functions easier? - but this is the general approach I think would work for this.

@danielbachhuber
Copy link
Contributor

I think the function signature of the callback function probably needs some thought as to possible use cases

Seems like a lot of code to register a callback function -_- Not sure how we'd do any better though, given it's a PHP API to register some frontend functionality.

Rather than add a callback argument, which restricts us to only one named callback, how do you feel stealing some of the proposal for JS actions in core (here's the latest)?

@goldenapples
Copy link
Contributor Author

Rather than add a callback argument, which restricts us to only one named callback, how do you feel stealing some of the proposal for JS actions in core (here's the latest)?

I actually like that concept a lot. There's no reason to have to name a callback function in the PHP array that registers the shortcode UI. Every update could trigger a named hook, like wp.hooks.shortcodeUi.img.linkto, and anything registered on that hook would be run. I'll give that approach a shot.

@danielbachhuber
Copy link
Contributor

Every update could trigger a named hook, like wp.hooks.shortcodeUi.img.linkto, and anything registered on that hook would be run.

Yes, this would be pretty nice.

cc @gcorne @azaozz if you have any opinions about how we experiment with this

@danielbachhuber
Copy link
Contributor

This uses a version of WP-JS-Hooks to provide a set of actions that
update callback functions can be hooked onto, rather than the rather
clumsy approach of having to register a function name in the PHP array
used to register the shortcode UI.

Code for the EventManager was taken basically wholesale from
https://github.com/carldanley/WP-JS-Hooks , but namespaced to
`wp.shortcodeUi.hooks` rather than `wp.hooks`, so as not to conflict
with the set of hooks that may land in core, in case there are slight
differences in the final API.
@goldenapples
Copy link
Contributor Author

Yeah, that pretty much jived with what I saw looking into those libraries. I used a slightly modified version of https://github.com/carldanley/WP-JS-Hooks, and registering a listener function makes a lot more sense now. Updated the branch in Image Shortcake that uses this; it hasn't changed much, but it's nice that javascript functions don't have to declare themselves in PHP anymore.

I'm not sure what the best way of including this code in the plugin is, though. I ended up including the WP Hooks EventManager class in the /lib directory and enqueuing it before and as a dependency of shortcode-ui.js. I can't think of a better way right now of including it that would indicate that it's external code, and that it should be kept up-to-date with developments in that external code.

@danielbachhuber
Copy link
Contributor

I'm not sure what the best way of including this code in the plugin is

What about:

  1. Putting it in lib/wp-js-hooks/wp-js-hooks.js
  2. Modifying the file so it assigns itself to wp.shortcake.hooks or similar (e.g. avoid conflict with more generic wp.hooks).
  3. Enqueue it with the last updated date as the version (e.g. 2015-03-19), so we have that as reference.

Would this address your reservations?

@goldenapples
Copy link
Contributor Author

Yeah, that's pretty much what I did here. The process is kind of a mess, but I couldn't come up with anything better. I considered forking the wp-js-hooks repo, modifying it as we needed, and registering it as a bower component, so that there would be a VCS trail back to the original project. But... the feature plugin project itself isn't really what I'd be interested in tracking here, so much as the core discussion. So referencing the trac ticket in that file seemed more useful.

@danielbachhuber
Copy link
Contributor

Cool. If you'd like to update then, we can land it.

* Removed accidental `console.log`.
* Changed the hooks' namespace from `wp.shortcodeUi` to `wp.shortcake`.
  Avoiding unnecessaryCamelCasing leads to greaterHappinessAllAround.
* Included a minified version of the wp-js-hooks file.
* Moved the wp-js-hooks script into its own directory for better
  organizational structure.
@goldenapples
Copy link
Contributor Author

Nice! Ready for final review, I think...

Originally I tried including wp-hooks as a requireJS module. That didn't
make any sense because it needs to be in the global namespace. So,
removed it.

Also, fixed the namespace in the action call from edit-attribute field
(the namespace registration was updated from `shortcodeUi` to
`shortcake` in the last commit, but editAttributeField was still trying
to call `wp.shortcodeUi.hooks.addAction`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we enqueue as shortcode-ui-js-hooks to avoid collisions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what is that version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we enqueue as shortcode-ui-js-hooks to avoid collisions?

Yeah, makes more sense. I'll update.

And what is that version?

The datetime I added that line. I guess that doesn't clear anything up at all - the chance that someone who updated the file would also remember to update the version number here is slim to none. Maybe better to just use the plugin version number like aberything else does?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use 2015-03-19 to more clearly indicate it's a date?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. What date does that refer to? The patch file I was building off of is from 2014-03-08.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the latest from the repo: https://github.com/carldanley/WP-JS-Hooks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, that makes sense. I've updated that, and clarified in the file header comments.

@danielbachhuber
Copy link
Contributor

Couple small questions.

The latest version of this interface doesn't require a callback
function. It doesn't need to be referenced in the specs anymore.
Avoids potential naming conflict with core functionality.
Updates the wp-hooks event manager to the version in the Github repo.
Uses the last modified date of the src file in the repo as the version
number for script enqueueing.
@danielbachhuber danielbachhuber added this to the v0.4.0 milestone Jun 1, 2015
danielbachhuber pushed a commit that referenced this pull request Jun 2, 2015
Add basic API for registering callbacks on attributes
@danielbachhuber danielbachhuber merged commit 16afde7 into master Jun 2, 2015
@danielbachhuber danielbachhuber deleted the 316-attribute-update-events branch June 2, 2015 13:37
danielbachhuber added a commit that referenced this pull request Jun 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants