Fix repeater collapsible on push #1724
Conversation
…ud/skyux2 into fix-repeater-collapsible-on-push
Codecov Report
@@ Coverage Diff @@
## master #1724 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 409 409
Lines 8486 8488 +2
Branches 1241 1242 +1
=======================================
+ Hits 8485 8487 +2
Misses 1 1
Continue to review full report at Codecov.
|
| @@ -1,8 +1,9 @@ | |||
| import { Component } from '@angular/core'; | |||
| import { Component, ChangeDetectionStrategy } from '@angular/core'; | |||
There was a problem hiding this comment.
Here (and elsewhere), imports need to be in alphabetical order, and on a new line.
See: #1647 (comment)
| @@ -8,6 +8,7 @@ import { skyAnimationSlide } from '../animation/slide'; | |||
| import { SkyRepeaterService } from './repeater.service'; | |||
| import { SkyLogService } from '../log/log.service'; | |||
| import { SkyCheckboxChange } from '../checkbox/checkbox.component'; | |||
| import { BehaviorSubject } from 'rxjs/BehaviorSubject'; | |||
There was a problem hiding this comment.
Third-party imports should be listed before "local" imports.
#1647 (comment)
| @@ -62,6 +53,17 @@ export class SkyRepeaterItemComponent { | |||
| this.slideForExpanded(false); | |||
| } | |||
|
|
|||
| public setIsCollapsible(value: boolean) { | |||
There was a problem hiding this comment.
To avoid a breaking change, we need to maintain the setter format. What's the reasoning behind the change?
public set isCollapsible(value: boolean) {
// ...
}
There was a problem hiding this comment.
The issue with the setter/getter was that they couldn't be different types. I wanted the getter to return an Observable but the setter would have to take in a boolean to set that. I could make the Observable property public or surface a separate getter for the observable version.
There was a problem hiding this comment.
for now I'm changing it to the latter
| @@ -34,22 +47,25 @@ export class SkyRepeaterItemComponent { | |||
|
|
|||
| public slideDirection: string; | |||
|
|
|||
| public get isCollapsible(): boolean { | |||
| return this._isCollapsible; | |||
| public get isCollapsibleObservable(): Observable<boolean> { | |||
There was a problem hiding this comment.
We've been trying to avoid Observables whenever we can because they add a layer of complexity that's difficult to debug later on. I was able to get this working without Observables, if I changed this component's change detection strategy to OnPush and used this.changeDetector.markForCheck() in the updateForExpanded() method.
Curious if that works on your end?
There was a problem hiding this comment.
Yeah, that does work. That is a cleaner implementation
| }) | ||
| export class SkyRepeaterDemoComponent { | ||
| public items: any[]; | ||
| public expandMode = 'single'; | ||
| public expandMode = 'none'; |
There was a problem hiding this comment.
We should probably keep this as 'single' to demonstrate the collapsible feature.
| @@ -1,13 +1,25 @@ | |||
| import { | |||
| Component, | |||
| Input | |||
| Input, | |||
| ChangeDetectorRef | |||
There was a problem hiding this comment.
Mind ordering these imports alphabetically?
| import { | ||
| skyAnimationSlide | ||
| } from '../animation/slide'; | ||
| import { |
There was a problem hiding this comment.
I know these imports were incorrectly formatted to begin with, but could we order them from external to local files? Also, each import needs a newline after it:
#1647 (comment)
import {
skyAnimationSlide
} from '../animation/slide';
import {
SkyCheckboxChange
} from '../checkbox/checkbox.component';
import {
SkyLogService
} from '../log/log.service';
import {
SkyRepeaterService
} from './repeater.service';
No description provided.