WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#33657 closed enhancement (wontfix)

wp.customize.Container.onChangeActive uses "construct" variable name instead of "container"

Reported by: ericlewis Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords:
Focuses: javascript Cc:

Description

r32743 introduced the local variable construct into wp.customize.Container.onChangeActive. Other functions in the class (e.g. the initialize method) use container when referring to an object instance. Can we do the same here for consistency?

Attachments (1)

33657.diff (1.8 KB) - added by ericlewis 5 years ago.

Download all attachments as: .zip

Change History (7)

@ericlewis
5 years ago

#1 @ericlewis
5 years ago

  • Summary changed from wp.customize.Container.onChangeActive uses "construct" var instead of "container" to wp.customize.Container.onChangeActive uses "construct" variable name instead of "container"

#2 @westonruter
5 years ago

I think I wanted to use construct as opposed to container because the latter is often used to represent the DOM container element. So using construct was supposed to be less confusing. See also the focus function:

focusContainer = construct.container.find( 'ul:first' );

If we changed it from construct to container then this would be: container.container

#3 @ericlewis
5 years ago

Alternatively, we could use this for using execution context, and consider adding that to the JavaScript coding standards.

I know there's a pattern of using the name of the class as execution context in Customizer JS (e.g. container for api.Container), so there's a preference here I don't want to dismiss outright.

However, I'll make the case for this. this is a native construct to the language, so you can open up a random object or class method and understand what this is with no pretext or research.

Using this would avoid awkward duplication in the case of this ticket:

// this line is awkward
focusContainer = container.container.find( 'ul:first' );
// this line reads more clearly
focusContainer = this.container.find( 'ul:first' );

For sharing execution context across scopes (e.g. inside a nested callback function), I don't have a particular preference. var self = this is common, but as this is not a native part of the language, everything is convention.

This ticket was mentioned in Slack in #core by eric. View the logs.


5 years ago

#5 @westonruter
5 years ago

@ericlewis: There is a case against against using this or self (or that), see #32911:

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.

/via @wonderboymusic

#6 @ericlewis
5 years ago

  • Keywords has-patch removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

It sounds like there's not much consensus here, and this isn't a serious issue.

Note: See TracTickets for help on using tickets.