Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#32911 closed task (blessed) (fixed)

Review Customizer Nav Menus JS

Reported by: wonderboymusic's profile wonderboymusic Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords:
Focuses: javascript Cc:

Description

A few notes:

  • The $.Deferred is unnecessary - when resolved, it just calls init, which adds another callback to the stack, which then gets called next. All of these can be combined into one.
  • self is a bad local variable name. Sometimes self is a substitute for this when scope is lost in a maze of callbacks, but most of the time, it just makes this more ambiguous than it already is.
  • Almost all of these functions are static helper methods, do any of them need to exposed publicly? We have gotten into a bad habit of globalizing all of our JS, which has made BC a living hell when we want to use things like browserify. I would expose as little as possible everywhere.
  • We should distinguish between settings and class/instance properties.

The patch makes this JS look more like the other Customizer JS. I am not going to drop it in right now. I may have to wait until all of the Customizer turbulence is over and then do one cleanup.

Attachments (1)

reboot.diff (13.3 KB) - added by wonderboymusic 9 years ago.

Download all attachments as: .zip

Change History (10)

#1 @wonderboymusic
9 years ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned

#2 @westonruter
9 years ago

  • Almost all of these functions are static helper methods, do any of them need to exposed publicly? We have gotten into a bad habit of globalizing all of our JS, which has made BC a living hell when we want to use things like browserify. I would expose as little as possible everywhere.

By exposing these publicly, we can unit test them (which still needs to be done, per #32688). So that's an advantage.

  • self is a bad local variable name. Sometimes self is a substitute for this when scope is lost in a maze of callbacks, but most of the time, it just makes this more ambiguous than it already is.

Agreed. We should use setting, control, or some other name that is more semantic.

#3 @westonruter
9 years ago

  • The $.Deferred is unnecessary - when resolved, it just calls init, which adds another callback to the stack, which then gets called next. All of these can be combined into one.

I've eliminated this in the patch for #32894.

#4 @westonruter
9 years ago

In 33134:

Customizer: Defer listening to nav menu setting changes until active.

Defer the preview starting to listen to setting changes until after the Customizer has synced settings into the preview. This ensures that any differences between the JS and PHP representations of the settings won't cause an infinite refresh.

See #32911.
Fixes #32894.

#5 @obenland
9 years ago

  • Type changed from defect (bug) to task (blessed)

#6 @obenland
9 years ago

Is there anything left to do here @wonderboymusic, or can it be closed?

#7 @wonderboymusic
9 years ago

In 33345:

Customizer: Nav Menus JS cleanup, first round

  • In init, settings should be fresh and set to default values before extending
  • When looping via _.each, pass this as 3rd arg to bind context
  • Settings should be encapsulated in the module, instead of being exposed as static class props
  • currentRefreshMenuInstanceDebouncedCalls should be encapsulated and renamed, instead of being the longest symbol in the entire codebase

See #32911.

#8 @wonderboymusic
9 years ago

In 33347:

Customizer: Nav Menus JS cleanup, second round

  • Follow the same pattern of namespace instantiation that WidgetCustomizerPreview uses
  • Remove use of self for global delegation
  • Use api for wp.customize and import only top-level globals
  • Bind this where appropriate and disambiguate scope

See #32911.

#9 @wonderboymusic
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.