Opened 9 years ago
Closed 9 years ago
#32911 closed task (blessed) (fixed)
Review Customizer Nav Menus JS
Reported by: | wonderboymusic | Owned by: | 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. Sometimesself
is a substitute forthis
when scope is lost in a maze of callbacks, but most of the time, it just makesthis
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)
Change History (10)
Note: See
TracTickets for help on using
tickets.
By exposing these publicly, we can unit test them (which still needs to be done, per #32688). So that's an advantage.
Agreed. We should use
setting
,control
, or some other name that is more semantic.