[wp-trac] [WordPress Trac] #32417: Add new core media widget

[WordPress Trac]

#32417: Add new core media widget
-------------------------------------------------+-------------------------
 Reporter:  melchoyce                            |       Owner:  melchoyce
     Type:  feature request                      |      Status:  assigned
 Priority:  normal                               |   Milestone:  4.8
Component:  Widgets                              |     Version:  4.3
 Severity:  normal                               |  Resolution:
 Keywords:  needs-unit-tests has-patch needs-    |     Focuses:  ui,
  refresh                                        |  administration
-------------------------------------------------+-------------------------

Comment (by sirbrillig):

 Nice work pushing this along! I did a quick scan of the JS code for the
 widget. It looks good! I do have a few mostly minor comments:

 - Why does binding the click event for the media image have its own click
 handler (`frame.bindImageClick`) and the click event for the button does
 not? Presumably because the image needs to be re-bound during
 `renderFormView`, but we might as well be consistent.
 - Similarly, why does the button have its selector stored in an object
 property, but the image does not? Presumably this is because the button
 selector is used elsewhere, but again it might be worth having both be
 properties or, if the selectors should never be overwritten, just use
 accessor methods to get them; something like `getMediaButton().on(...)`
 and `getMediaImage().on(...)`.
 - Why is the reference to `widgetFrame` stored at the top-level of the
 IIFE rather than within `openMediaManager`? It's not used anywhere else
 and it's always overwritten when `openMediaManager` is called.
 - I don't know much about `wp.media`, but is there a possible memory leak
 since we are binding functions to the events `open` and `select` and then
 re-creating the object each time the frame is made visible? Or are those
 event handlers automatically removed when the media frame is hidden?
 - This line `ids = $( '#widget-' + widgetId + '-id' ).val().split( ',' );`
 will throw an error if the target cannot be found on the page. How about
 something like `idGroup = $( '#widget-' + widgetId + '-id' ).val() || '',
 ids = idGroup.split( ',' );`?
 - This conditional is confusing to me: `if ( ids[ 0 ] > 0 ) {
 ids.forEach(`. Why check the integer value of the first element of the
 widget, then proceed to iterate over the array? Could we just iterate over
 the array regardless? If the concern is that we should ignore `0` IDs,
 could we just ignore them in the loop?
 - Missing a `@return` jsdoc line for `renderMediaElement`.
 - The `translate()` function uses `window._mediaWidgetl10n` followed by a
 bare reference to `_.medaiWidgetl10n`. It seems like we should be
 consistent here. I suggest passing `window._mediaWidgetl10n` as an
 argument to the IIFE so it can be used without `window` inside.
 - `renderFormView` uses jQuery to find `formView` and `scale` but doesn't
 account for the possibility that those elements might not be found. Should
 we return early?
 - Although it is clever, using a ternary property reference to call the
 appropriate class manipulation method is not obvious to me and may confuse
 other developers.  `formView.find( '.attachment-description' )[
 attachment.description ? 'removeClass' : 'addClass' ]( 'hidden' )` could
 instead be written as `formView.find( '.attachment-description'
 ).toggleClass( 'hidden', ! attachment.description ).html(
 attachment.description );`
 - Since we are using underscore.js anyway, why not use `_.contains()`
 instead of `$.inArray()` since it already returns true/false, removing the
 need to compare to `-1`?
 - `else if` can sometimes be confusing when trying to follow the flow of
 code, leading to hard-to-find bugs. Could we replace it with a plain
 condition, so that `} else if ( 'image' === attachment.type ) {` becomes
 `if ( ! _.contains( [ 'audio', 'video' ], attachment.type ) && ( 'image'
 === attachment.type ) ) {`?
 - Could each media type in `renderMediaElement` be its own function to
 make that function smaller and easier to read? We could even use another
 function to figure out which one to call; something like `return
 getMediaRendererFor( attachment.type )( widgetId, props, attachment )`.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/32417#comment:104>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform