[WIP] Widget JS Refactor #17
Conversation
It turns out that the caption, alt text, and other fields do need to be stored with the widget
[ci skip]
|
@obenland FYI: Here's the progress I've made so far on refactoring the JS to be extensible, with a base |
|
This is quite a PR. Are you still working on it? Do you think this could be split up in smaller PRs? |
wp-admin/js/widgets/media-widgets.js
Outdated
|
|
||
| $( document ).on( 'widget-added', function( event, widgetContainer ) { | ||
| console.info( widgetContainer ) | ||
| var widgetContent, controlContainer, widgetForm, widgetId, idBase, ControlConstructor, ModelConstructor, modelAttributes, control, model; |
There was a problem hiding this comment.
Is the order of variables organized or random?
There was a problem hiding this comment.
This whole function is a mess and needs further cleanup.
Unfortunately, not. It can't be split up because it's a rewrite of the JS. I am still working on it, and hope to have the changes I want to make in place be EOD today. But we should probably merge it once the image widget is completed, and then open new PRs for the audio and video widgets, do you think? |
|
Is there any way I can contribute or help? Everything else seems blocked until this gets in |
| var control = this, previewContainer, previewTemplate; | ||
| previewContainer = control.$el.find( '.media-widget-preview .rendered' ); | ||
| previewTemplate = wp.template( 'wp-media-widget-image-preview' ); | ||
| previewContainer.html( previewTemplate( { attachment: control.selectedAttachment.attributes } ) ); |
There was a problem hiding this comment.
Is there a reason control.selectedAttachment.attributes is not passed straight to the template?
There was a problem hiding this comment.
@obenland Good point. Maybe not, although maybe this is anticipating the ability to pass in a url separately in the case of providing an external image by URL instead of from the media library.
| wp_json_encode( $this->widget_options['mime_type'] ), | ||
| wp_json_encode( $this->l10n ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
What made you decide to use inline scripts rather than wp_localize_script() and assigning its values?
There was a problem hiding this comment.
@obenland yes, by using wp_add_inline_script() it means we can avoid having to create a new global variable and can instead amend an existing object directly. Also, wp_localize_script() will cast scalar array values to strings which may not always be desired.
There was a problem hiding this comment.
Avoiding a global variable doesn't come free. It makes media-image-widget.js that much harder to grok for contributors who are unfamiliar with the code
Work in progress
This PR will introduce a lot of churn but it will give a much more extensible foundation for us to extend new media widget.
Fixes #8
Fixes #7
Fixes #16