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(slide-toggle): invalid model change event #4220

Merged
merged 3 commits into from Jul 28, 2017
Merged

fix(slide-toggle): invalid model change event #4220

merged 3 commits into from Jul 28, 2017

Conversation

devversion
Copy link
Member

While initially looking into #4124, there were a few more issues inside
of the slide-toggle.

  • ngModelChange event is dispatched at initialization.

  • Checked state isn't synchronized when state changes through drag. New state after dragging got overwritten by click event on label.

  • Removes unnecessary logic inside of change listener. Change event doesn't fire if underlying checkbox is disabled.

Fixes #4124.

@devversion devversion added the in progress This issue is currently in progress label Apr 22, 2017
@devversion devversion self-assigned this Apr 22, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 22, 2017
@devversion devversion added pr: needs review and removed in progress This issue is currently in progress labels Apr 22, 2017
@devversion devversion assigned kara and unassigned devversion Apr 22, 2017
@devversion devversion requested a review from kara April 22, 2017 23:09
@jelbourn jelbourn requested review from tinayuangao and removed request for kara April 24, 2017 16:22
@devversion
Copy link
Member Author

Ah sorry, didn't assign @tinayuangao for review here, because @kara reverted the PR before 😄

@devversion
Copy link
Member Author

@tinayuangao Can you review this again? This basically is the old PR just with the Cast NgModel value to boolean commit (that should fix the internal google issues)

@tinayuangao
Copy link
Contributor

LGTM

@devversion
Copy link
Member Author

@tinayuangao Thanks. @kara I'm going to mark this as merge ready now. Let me know if there are any failures again.

@devversion devversion added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels May 5, 2017
@kara kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label May 9, 2017
@jelbourn
Copy link
Member

jelbourn commented May 23, 2017 •
edited

Presubmit error is that one team's e2e tests are failing, but it's not clear why. We need to follow-up with that team.

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jun 9, 2017
@devversion
Copy link
Member Author

Rebased.

@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jun 12, 2017
@devversion devversion removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jun 17, 2017
@devversion
Copy link
Member Author

@tinayuangao @kara Can you review this? I removed lgtm because I changed a few things.

@devversion devversion assigned kara and tinayuangao and unassigned devversion Jun 17, 2017
kara
kara approved these changes Jun 19, 2017
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@kara kara removed their assignment Jun 19, 2017
@kara kara added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 19, 2017
@e-cloud e-cloud mentioned this pull request Jun 20, 2017
Closed
@mmalerba mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jun 21, 2017
@mmalerba
Copy link
Contributor

Please rebase

@devversion
Copy link
Member Author

@mmalerba Done!

@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jun 22, 2017
@mmalerba mmalerba added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jul 6, 2017
@e-cloud
Copy link
Contributor

e-cloud commented Jul 10, 2017

any progress?

@devversion
Copy link
Member Author

Rebased. I just need more information what's causing the presubmit failures. cc. @mmalerba

@devversion
fix(slide-toggle): invalid model change event (angular#4140)" (angula…
08699f1 
…r#4218)

While initially looking into angular#4124, there were a few more issues inside of the slide-toggle.

* ngModelChange event is dispatched at initialization.

* Checked state isn't synchronized when state changes through drag. New state after dragging got overwritten by click event on label.

* Removes unnecessary logic inside of `change` listener. Change event doesn't fire if underlying checkbox is disabled.

Fixes angular#4124.
@devversion
Cast NgModel value to a boolean
0365c7a
@devversion
Properly mark component for check
36dea28
@andrewseguin andrewseguin merged commit dfe10c3 into angular:master Jul 28, 2017
@devversion devversion deleted the fix/slide-toggle-drag branch November 11, 2017 10:22
@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

@kara kara kara approved these changes

@tinayuangao tinayuangao tinayuangao approved these changes

Assignees

@tinayuangao tinayuangao

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

slide-toggle fires undesired event when setting a default value
8 participants
@devversion @tinayuangao @jelbourn @kara @mmalerba @e-cloud @googlebot @andrewseguin