Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#28965 closed defect (bug) (fixed)

Media Grid JavaScript Clean-up

Reported by: ericlewis's profile ericlewis Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch
Focuses: javascript, docs Cc:

Description

Media Grid has been developed in a flurry.

Let's sweep through and fix up where urgency previously trumped cleanliness.

Attachments (3)

28965.diff (24.5 KB) - added by ericlewis 10 years ago.
28965.2.diff (29.7 KB) - added by ericlewis 10 years ago.
28965.3.diff (478 bytes) - added by ericlewis 10 years ago.

Download all attachments as: .zip

Change History (17)

@ericlewis
10 years ago

#1 @ericlewis
10 years ago

In attachment:28965.diff:

  • Instead of using an explicitly stated CSS class at the top-level frame of media grid,

use the mode-grid selector to apply styles.

  • Remove the edit mode, which is cruft from a different implementation of bulk edit.
  • Remove screen options CSS, which is unnecessary since the grid is not styled fixed.
  • Use more self-documenting function names, e.g. createViewForContentRegionInBrowseMode, which while verbose, is more useful than createBrowse
  • For media.controller.EditAttachmentMetadata state, remove unnecessary function overloading in lieu of setting region defaults to false in the options hash.
  • Move mode functionality from media.view.MediaFrame to media.view.Frame`.
  • Remove the custom events hash from media.view.Attachment.Details.TwoColumn. Closer method parity with media.view.Attachment.Details' events hash.
  • Remove unnecessary methods: media.view.Attachment.Details.render
  • Fix up the image editor view loading business in media.view.MediaFrame.EditAttachments
Version 1, edited 10 years ago by ericlewis (previous) (next) (diff)

@ericlewis
10 years ago

#2 @ericlewis
10 years ago

In attachment:28965.2.diff:

  • More documentation.
  • Replace media.view.MediaFrame.EditAttachments.editMetadataContent with media.view.MediaFrame.EditAttachments.createViewForContentRegionInEditMetadataMode (self-documenting method naming), and clean up the implementation.

#3 @ericlewis
10 years ago

  • Keywords has-patch added

#4 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 4.0

#5 @DrewAPicture
10 years ago

  • Focuses docs added

#6 follow-up: @wonderboymusic
10 years ago

In 29266:

Media Grid: general JS cleanup.

Props ericlewis.
See #28965.

#7 @wonderboymusic
10 years ago

I left out the iOS-style method name changes for now - I think better docs would alleviate the need for those.

#8 in reply to: ↑ 6 @afercia
10 years ago

Replying to wonderboymusic:

In 29266:
Media Grid: general JS cleanup.

hi,
after this I get a blank media grid, no grid items at all, no js errors in the console. I have to add back "router: false" to the default states in media-grid.js but I'm totally new to backbone.js sorry I can't help more.

#9 @wonderboymusic
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

a lot was tackled here

@ericlewis
10 years ago

#10 @ericlewis
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In attachment:28965.3.diff: per @koop, don't select body with jQuery.

#11 @wonderboymusic
10 years ago

In 29367:

Media Grid: don't select body with jQuery when instantiating media.view.UploaderWindow.

Props ericlewis.
See #28965.

#12 @koop
10 years ago

For clarity's sake: document.body is the body element itself, not a jQuery collection. In this case, it'll happen to work either way (which I dislike, I'd rather a more specific API).

Also, I think the container property might be vestigial at this point.

(Also, hey everyone.)

#13 @DrewAPicture
10 years ago

  • Summary changed from Media Grid Javascript Clean-up to Media Grid JavaScript Clean-up

#14 @wonderboymusic
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

This ticket, like #24716, is too broad at this point. We should open tickets for specific things we are doing, or just reference other tickets that we are expanding upon when committing.

Note: See TracTickets for help on using tickets.