Rework attachment field view so that it triggers events #358
Conversation
Just restructures this attribute field model a bit to better follow the general pattern of Backbone views.
Abstracts triggerCallbacks to its own method, so it can be called from attribute fields that extend `editAttributeField` but override its `updateValue` method.
I can't see any reason to build in an extra 10K of scripts into this file which are already being included elsewhere on the page just for the purpose of encapsulation. I tried to remove all the browserify dependencies and set this field up as a model for how plugin users could register their own fields in a more typical Backbone view structure. I like it a lot better. I am exposing one additional global, the `iDCache` variable, and that's somewhat intentional - I'd like for filters on updating attachment to have access to that data to avoid having to make additional ajax calls to look it up. The variable name and the way in which it's exposed is pretty ugly though, and I think it would do better as a public method on the view... Also, browserify is still processing this file, I'm just loading the version from `src` rather than the browserified `build` file. It would be good to come up with an organizational structure that made more sense for these view files.
|
@danielbachhuber @mattheu I'd love some feedback on this approach for registering attribute fields if either of you have a chance. I think all of the custom attribute fields - attachment, post select, and color picker - should probably be registered with a standalone view definition like this rather than built into unnecessarily large and complex browserified beasts. This seems like a simpler pattern to ask plugin developers to follow when developing their own UI for attribute fields. In the short term, this makes changes to the field like #350 and #351 pretty simple, and it makes listening for events as in #316 possible. |
|
I don't have a strong opinion one way or another at this point. Couple pieces of context:
However, I'm also hesitant to "fix something that isn't broken". As such, it makes sense to have a clear and compelling reason for the change. |
My impetus for this pull request is that
That is separate from the question of whether the script for these fields is built through Browserify. I just noticed that it was an unnecessary step. The only real interface the plugin offers, the Plus, if we make it possible to extend the default editAttributeField without requiring devs to bundle chunks of scripts from this plugin, then its possible to extend Shortcake for capabilities that don't have to be built into the plugin itself. Otherwise, developers are limited to the fields provided. I could well be missing something. @jitendraharpalani58 do you have any feedback here? |
I definitely agree with you. The reason for creating a global variable Although, If these custom fields are making their way into |
|
See issue #360. |
Following @jitendraharpalani's comments, it makes sense for this to be bundled into the main plugin js file and avoid the extra unnecessary HTTP request. It's still architected basically as a standalone view, which I think is a good thing. If we keep the pattern used for composing views fairly standard and standalone, it will be easier to give examples for extending these views. Everything needed to extend a view should already be accessible through extending the `sui.views.editAttributeField` field.
|
Following @jitendraharpalani58's comments, I moved the
|
Knowing that a field has updated seems less useful than actually retrieving the information from that field. This moves the event trigger into the ajax callback, so that it doesn't trigger until all of the attachment data has been retrieved.
We're no longer loading it separately, so it doesn't need to be browserified separately.
Setting the cache as a cache method and defining getter/setter helper functions for it makes it possible to get around using the sui.data fudge that I was using before.
I ended up just using class methods on the view to store the cache and get items from it or set items in it. Seems better to me. |
There was a problem hiding this comment.
What will the removal of this file break?
There was a problem hiding this comment.
Nothing. I'm not sure why Github is showing it as a removal, it's really just being moved to js/src/views/edit-attribute-field-attachment.js and built into the shortcode-ui.js bundle.
|
I don't have any strong opinions on the architectural changes. I'm happy to be a rubber duck as you talk through them though. It would be good to get @mattheu's input on this PR |
Added getValue() and setValue() instance methods to editAttributeField, and a getField() class method for finding a field in a collection.
Fix rather silly bug.
I specifically wrote a function called getValue() which takes no arguments and gets a value. Overloading it by allowing it to get other attributes seems like it breaks consistency. Also, the way I was doing it just didn't work.
|
OK, I'm starting to feel pretty good about this shape of this now. I've added a couple of helper methods for dealing with attribute field views: I also added a finder function, Finally, I added a class method to An example of a listener using these helper methods is at https://github.com/fusioneng/image-shortcake/pull/44/files @mattheu @danielbachhuber Do either of you want to review this or give me some feedback on the direction it's going? Now that the basic structure to extend is starting to fall into place, it should be simple to rework the other attribute field types so that they also inherit some of these helper methods and event triggers. |
|
👍 from me |
…ield Rework attachment field view so that it triggers events
See #316.
This is a start at rewriting the "attachment" field view as a Backbone view that extends
editAttributeField, and therefore has access to the actions defined in there. A big change is that itis no longer browserified andonly depends on the public variables exposed insui.views. I think this is a simpler approach and provides a better model to point people to for developing additional field types.Architecturally - still needs some organizational work. I wanted to put it out for general feedback first, though.