Components: Make all @Output names, property names, and event class names consistent

[jelbourn]

[amcdnl]

Here is my initial audit and proposals before I make any changes.

• Change names to past tense select to selected
• Remove prefixes from event names e.g. onSelect - Angular Style Guide

Chip

• select: Change to selected. Select is a native event emitted when text is selected by the cursor.
• deselect => deselected.
• destroy => destroyed
• onRemove => removed

Date Picker

• selectedChanged - @jelbourn any reason why this wouldn't just be changed?
• userSelection => userSelected
• dateChange => dateChanged
• selectedValueChange => changed?

Menu

• close => closed
• onMenuOpen => menuOpened or opened
• onMenuClose => menuClosed or closed

Paginator

• page => paged

Select

• onOpen => opened
• onClose => closed
• valueChange => valueChanged

Sidenav

• onOpen => opened
• onClose => closed
• onPositionChanged => positionChanged
• onAlignChanged => alignChanged

Sort

• mdSortChange => mdSortChanged

Tabs

• selectedIndexChange => selectedIndexChanged
• focusChange => focusChanged
• selectChange => selectChanged
• onCentering => centering
• onCentered => centered

@jelbourn - do we want to convert change to changed? Also there is several places that have changed prefixed like mdSortChange, do we want to consolidate those to change or would we like to go the other way and prefix everything?

[jelbourn]

• Are you going to do a separate pass for event class names? (e.g., radio buttons emit MdRadioChange but mdSort emits just Sort).
• Are there any places where the past-tense event name conflicts with another property or method in the class? I recall that being the reason for some members being onSomething
• Most of the changes seem fine, but some are off
  
  
  
  • The datepicker ones seem weird, cc @mmalerba. userSelected is definitely not right.
  
  
  • For tabs, why is focusChange a thing? Wouldn't some combination of focus/blur/focusin/focusout cover everything? Maybe that should be internal. cc @andrewseguin
  
  
• The _@Output_ names should be prefixed with mdSomething when the directive selector is camelCase. So for e.g. sort, the output name should be mdSortChanged
• The actual _class property_ names should all be unprefixed always. So you'd end up with, e.g.,

@Output('mdSortChanged')
changed = new EventEmitter<Whatever>();

cc @angular/angular-material since this affects everything

If we move forward with this, it would be good to do one PR per component; this will make it easier to get the changed into Google. For the more widely-used components, we'll probably need a window of backwards compatibility.

[mmalerba]

For datepicker, selectedChanged and userSelection are used on internal pieces that aren't really meant for users. Those outputs are to help us coordinate behavior among the various sub-components that make up the datepicker popup. selectedChanged is essentially just a de-duped version of userSelection so we can probably use a single Observable and do stream operations to de-dup.

Also dateChange and dateInput on the datepicker input were meant to mimic native input's standard change and input events. I don't know what's more important consistency with those native events or consistency with our other components, but just wanted to point this out.

[jelbourn]

We should mark those two with an _ then.

I'd be okay with dateChanged to be consistent with other components, but I'd leave dateInput (since "input" is acceptable for past-tense as well)

[crisbeto]

A note on the valueChange from md-select: that one has to stay as valueChange in order for the two-way binding for value to work. It is already marked as private and we can potentially underscore it.

[amcdnl]

@jelbourn -

1) Sure, I can do a separate pass for that. I wasn't really focusing class names in this but we should def do that too. I actually think they should have some prefix on them, you might end up in a situation where you import 2 different components and lets say they both have a 'Change' event now you would 2 conflicting names.
2) I think in the case where there is conflicts, it probably needs to be prefixed w/ a _ or better name. I did not check for conflicts during this pass.
3) As part of the pass, I looked at ALL event names not just externals. So are you suggesting if a event name is used internally but never propagated that we _ it?

On @crisbeto point, the 2 way binding relies on the input names having a suffix of change on them for it to work. We might want to reconsider the past tense given we would deviate from that and have 2 different naming conventions.

[jelbourn]

Yeah, if an event is only used by us (not meant to be a public API), then it should start with an underscore.

For the event class names, I'd think they should all be something like MdRadioChange, MdSortChange, etc.

In places where we add an output only for two-way binding, can we still get away with marking those as internal (underscore property name, @Output alias for the name that matches).

[amcdnl]

Are we good to move forward?

@jelbourn - Do you want these in distinct PRs for each component? Should I support backwards compatibility?

[mmalerba]

+1 to *Change instead of *Changed, shorter & works with 2-way binding syntax

[jelbourn]

Maybe saying always past-tense is overboard.

How about

Chip

• select: => selected
• deselect => deselected
• destroy => destroyed
• onRemove => removed

Date Picker

• selectedChanged - _selectedChanged (internal)
• userSelection => _userSelection (internal)
• dateChange
• dateInput
• selectedValueChange

Menu

• close => closed
• onMenuOpen => menuOpened
• onMenuClose => menuClosed

Paginator

• page

Select

• onOpen => opened
• onClose => closed
• valueChange

Sidenav

• onOpen => opened
• onClose => closed
• onPositionChanged => positionChange
• onAlignChanged => alignChange

Sort

• mdSortChange

Tabs

• selectedIndexChange
• focusChange => _focusChange (internal)
• selectChange => selectedTabChange
• onCentering => _onCentering (internal)
• onCentered => _onCentered (internal)

So every somethingChange stays the same.

[amcdnl]

My thoughts from the suggestions:

Chip

• Proposed selected is already used as a input. Possible alternatives could be: selectedChange, selectedChange, chipSelected, mdChipSelected. Using the change could leverage the 2 way binding standard.

Date Picker

• Proposed selectedValueChange feels like it should be Changed to stay w/ the pattern. Also, selectedValueChange might infer there is a selectedValue input based on the angular 2way binding pattern.

Paginator

• Proposed page feels like it should be paged. If we were to make it pageChange, we could switch pageIndex to just page and leverage the 2 way binding standard.

Select

• Proposed valueChange is already the name of the output. This might infer there is a value input though based on the angular 2-way binding naming standard.

Sidenav

• Proposed positionChange, should this be past tense? This also might infer position is 2-way binding naming.
• Proposed alignChange, should this be past tense? This also might infer align is 2-way binding naming.

Sort

• Proposed mdSortChange, this prefix standard isn't used anywhere else. How about sortChange?

Overall

I feel like using Change as a suffix might infer there is 2-way binding going on. On the other hand, using Change stays with the overall theme but it deviates from the past tense concept. My suggestion would be:

• If the name is referring to a value, it should always end in Change. So for example: selectedChange.
• If the name is referring to a action, it should always end in past tense since the action has already taken place. Example menuClosed.

If we take the approach for naming actions to past tense it differs in tense from the rule change suffix rule. In the Angular Style Guide rule I mentioned above, they use past tense in that example so I think its the right move but they don't follow that rule with internally w/ the change suffix rule.

[jelbourn]

We don't necessarily follow the Angular style guide- there's some stuff that I disagree with in there anyway.

• I would keep change as present-tense universally
• Any directive that uses camelCase properties needs a fully qualified prefix, e.g. mdSortChange. I think this is the only one right now because mdTooltip and mdTextAreaAutosize don't emit any events
• I would do selectionChange over selectedChange
• I'm not too concerned with people thinking there would be two-way binding; I'm skeptical most people even know that's how two-way binding is set-up.

[amcdnl]

@jelbourn -

• Re: Tabs focusChange seems to be a public API, are you sure you want to change that to _focusChange?
• Re: Sidenav opened is a input api. Would you like to move this to openChange and closeChange respectfully?
• Re: Select on the same note as above, opened is not currently implemented but I can see it as a possible input for this component too. Thoughts?
• Re: date picker selectedChanged is already deprecated for dateChange and dateInput.
• Re: calendar selectedChange is used for 2-way binding on the selected property. I can't change this without breaking that.

[jelbourn]

• AFAIK focusChange wasn't ever meant to be public vs. people just using focus, focusin, etc.
• @mmalerba should chime in on the sidenav one. Maybe just one event called openChange
• If select doesn't have an event for opened now, we don't need to add one right now
• For datepicker, dateChange and dateInput are good
• For calendar, I can live with that

[mmalerba]

I feel like open and close are kind of special case events that should be left how they are since its more intuitive to users. If we must change to the *Change style though, I prefer a single openChange event

[jelbourn]

@mmalerba how would you otherwise deal with opened being the boolean @Input as well?

We _could_ do something like

@Input() opened: boolean;

@Output() openChange = new EventEmitter<...>();

@Output('opened') get _openChangeOut() {
  return filter.call(this.openChange, c => c.opened);
}

@Output('closed') get _openChangeOut() {
  return filter.call(this.openChange, c => !c.opened);
}

[amcdnl]

@jelbourn - I've submitted all PRs for this.

[mmalerba]

Ah I see, I didn't realize you were trying to get 2-way binding to work. Ok in that case either opened and openedChange or open and openChange are fine by me

[savaryt]

Related:
Casing inconsistency
It should be either
MatSnackBar => MatSnackbar
or
MatToolbar => MatToolBar
for the two components to respect the same naming convention.

[angular-automatic-lock-bot[bot]]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{_This action has been performed automatically by a bot._}