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.

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

fix(slider): Fix style change for updated slider and directionality change #6700

Merged
merged 6 commits into from
Sep 1, 2017

Conversation

g1shin
Copy link

@g1shin g1shin commented Aug 29, 2017

  1. Before the fix: the slider style was not getting updated upon blur:
    slider-cd-issue
    Fixes [slider] change detection issue with styles #6542

  2. Fixes directionality change not re-rendering updated slider.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 29, 2017
@g1shin g1shin changed the title Fix focus change for updated slider fix(slider): Fix focus change for updated slider Aug 29, 2017
@mmalerba
Copy link
Contributor

I'm not sure this is the right fix, doesn't make sense to blur it on click

@g1shin g1shin changed the title fix(slider): Fix focus change for updated slider fix(slider): Fix style change for updated slider and directionality change Aug 30, 2017
@g1shin
Copy link
Author

g1shin commented Aug 30, 2017

Changes have been made to this PR. Ready for review now 👍

@g1shin
Copy link
Author

g1shin commented Aug 30, 2017

Changes made based on discussion. Ready for review again

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 30, 2017
@g1shin
Copy link
Author

g1shin commented Aug 30, 2017

I added a change to fix prerender test.

@kara kara removed their assignment Aug 31, 2017
@g1shin
Copy link
Author

g1shin commented Aug 31, 2017

Moved the subscription to directionality change into ngOnInit() as well

@g1shin
Copy link
Author

g1shin commented Aug 31, 2017

Moved other subscriptions as well and removed _initialized

@@ -410,13 +410,25 @@ export class MdSlider extends _MdSliderMixinBase
private _changeDetectorRef: ChangeDetectorRef,
@Optional() private _dir: Directionality) {
super(renderer, elementRef);
}

ngOnInit() {
Copy link
Contributor

Choose a reason for hiding this comment

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

class should implement OnInit

.subscribe((origin: FocusOrigin) => this._isActive = !!origin && origin !== 'keyboard');
.monitor(this._elementRef.nativeElement, this._renderer, true)
.subscribe((origin: FocusOrigin) => {
this._isActive = !!origin && origin !== 'keyboard';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indent like this:

this...
    .monitor(...)
    .subscribe(...
      ...
    })

@g1shin
Copy link
Author

g1shin commented Aug 31, 2017

Comments have been addressed 👍

@jelbourn jelbourn merged commit 8c49422 into angular:master Sep 1, 2017
@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.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[slider] change detection issue with styles
6 participants