Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(select): make @Output names consistent #6677 #7160

Closed
wants to merge 6 commits into from
Closed

fix(select): make @Output names consistent #6677 #7160

wants to merge 6 commits into from

Conversation

amcdnl
Copy link
Contributor

@amcdnl amcdnl commented Sep 19, 2017

Updated APIs per #6677

  • onOpen to opened
  • onClose to closed
  • change to selectionChange

@amcdnl amcdnl self-assigned this Sep 19, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 19, 2017
@jelbourn jelbourn changed the title chore(api): update output api #6677 fix(select): update output api #6677 Sep 19, 2017
jelbourn
jelbourn reviewed Sep 19, 2017
View reviewed changes
src/lib/select/select.ts Outdated
@@ -402,13 +402,31 @@ export class MdSelect extends _MdSelectMixinBase implements AfterContentInit, On
}

/** Event emitted when the select has been opened. */
@Output() onOpen: EventEmitter<void> = new EventEmitter<void>();
@Output() opened: EventEmitter<void> = new EventEmitter<void>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this consistent with sidenav and just have one openedChange?
Thoughts @kara @crisbeto?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine by me, we just have to make sure to mark it as a breaking change.

crisbeto
crisbeto reviewed Sep 19, 2017
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise LGTM, pending a decision on what to do with the openedChange.

@jelbourn jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 22, 2017
@amcdnl
Copy link
Contributor Author

amcdnl commented Sep 24, 2017

@jelbourn - let me know what you want to do regarding that.

@jelbourn
Copy link
Member

If we're making the sidenav openChange then we should do that here too

@amcdnl
Copy link
Contributor Author

amcdnl commented Sep 30, 2017

@jelbourn - should be good to go now.

@crisbeto crisbeto mentioned this pull request Oct 15, 2017
Merged
jelbourn
jelbourn approved these changes Oct 24, 2017
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 24, 2017
@jelbourn
Copy link
Member

jelbourn commented Oct 24, 2017 •
edited
Loading

Caretaker note: should be completely backwards compatible, so if something fails there's a bug

@amcdnl amcdnl changed the title fix(select): update output api #6677 fix(select): make @Output names consistent #6677 Oct 25, 2017
@amcdnl amcdnl mentioned this pull request Oct 26, 2017
Merged
@amcdnl
Copy link
Contributor Author

amcdnl commented Oct 26, 2017

closing in favor of #8052

@amcdnl amcdnl closed this Oct 26, 2017
@amcdnl amcdnl deleted the select-outputs-6677 branch October 26, 2017 18:59
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@jelbourn jelbourn jelbourn approved these changes

@kara kara Awaiting requested review from kara

Assignees

@amcdnl amcdnl

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amcdnl @jelbourn @crisbeto @googlebot