feat(accordion): add animations #3766

fbasso · 2020-06-11T08:22:36Z

Cherry picked from #2817, with some changes.

The key points are:

Animations are managed one panel at a time, in the original code, a loop was done on all panels to guess if an animation is required, for each panel. The code is simpler this way, and aligned with ngbTransition API,
ngbCollapseChange event have been added. It follows the collapse widget, to discuss if the name/API must be changed,
A little change in ngbTransition has been required, to provide the context when animations are deactivated,
Tests have been done the same way the collpase widget,
Some detectChanges have been added in the previous tests, because of the changeDetector.detectChanges() required in _runTransition,
Only one commit in this PR, as there are only a few impacted files.

codecov-commenter · 2020-06-11T08:46:21Z

Codecov Report

Merging #3766 into animations will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           animations    #3766      +/-   ##
==============================================
+ Coverage       91.78%   91.82%   +0.04%     
==============================================
  Files             107      107              
  Lines            3127     3145      +18     
  Branches          565      566       +1     
==============================================
+ Hits             2870     2888      +18     
  Misses            189      189              
  Partials           68       68

Flag	Coverage Δ
#e2e	`55.23% <19.04%> (-0.23%)`	⬇️
#unit	`88.93% <100.00%> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
src/accordion/accordion-config.ts	`100.00% <100.00%> (ø)`
src/accordion/accordion.ts	`97.56% <100.00%> (+0.63%)`	⬆️
src/util/transition/ngbTransition.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e941d7...a3a11bf. Read the comment docs.

maxokorokov

Hey, @fbasso!

Thanks, this is shorter than before! Left some comments.

first let's agree on the event API, I have my thoughts in the comment below
add tests for collapsing events with animations = false
check the payload of those events in tests
I think we can remove zone.onStable and all double fixture.detectChanges() from tests
don't run detectChanges() N times for N panels
Either remove the fix for the ngbRunTransition or add tests

Cheers,
Max

src/accordion/accordion.spec.ts

maxokorokov · 2020-06-18T08:26:33Z

src/accordion/accordion.ts

+  /**
+   * If `true`, accordion will be animated.
+   */
+  @Input() animation = false;


Please remove = false

This is still not fixed

src/accordion/accordion.ts

src/util/transition/ngbTransition.ts

src/accordion/accordion.ts

maxokorokov

Hey, @fbasso!

LGTM for the code!
The comments I have mostly test- and documentation-related.

I guess that would be the last batch:

Please check shown/hidden is called in the tests (you're still not doing it when animations enabled)
Please remove animation = false in the NgbAccordion (you forgot to do it form the last review)
Please add comments in the code for "tricky" parts (ex. why do we need change detection and why we restart transition even thought it might already be running)

some other minor comments.

maxokorokov · 2020-06-24T09:22:41Z

src/accordion/accordion.spec.ts

+      reduceMotion = true;
+      onShown = (panelId) => panelId;
+      onHidden = (panelId) => panelId;
+      onFirstPanelShown = () => {};


you're not using this in ANY tests, please add checks that these callbacks are actually called

you could have 1 function and just check what id it is called with:

onPanelShown = () = {};

(shown)="onPanelShown('one')"

Finally, I need only the check on panel one, so I'll use it without parameter.

maxokorokov · 2020-06-24T09:23:04Z

src/accordion/accordion.spec.ts


    // Select the first tab -> change event
    getButton(fixture.nativeElement, 0).click();
    fixture.detectChanges();
    expect(fixture.componentInstance.changeCallback)
        .toHaveBeenCalledWith(jasmine.objectContaining({panelId: 'one', nextState: true}));
+    expect(fixture.componentInstance.shownCallback).toHaveBeenCalledWith('one');
+    expect(fixture.componentInstance.panelShownCallback).toHaveBeenCalled();


could we check with .toHaveBeenCalledWith(undefined) instead?

maxokorokov · 2020-06-24T09:23:09Z

src/accordion/accordion.spec.ts


    // Select the first tab again -> change event
    getButton(fixture.nativeElement, 0).click();
    fixture.detectChanges();
    expect(fixture.componentInstance.changeCallback)
        .toHaveBeenCalledWith(jasmine.objectContaining({panelId: 'one', nextState: false}));
+    expect(fixture.componentInstance.hiddenCallback).toHaveBeenCalledWith('one');
+    expect(fixture.componentInstance.panelHiddenCallback).toHaveBeenCalled();


could we check with .toHaveBeenCalledWith(undefined) instead?

maxokorokov · 2020-06-24T09:24:01Z

src/accordion/accordion.spec.ts

@@ -552,25 +555,37 @@ describe('ngb-accordion', () => {
    expect(panelContents.length).toBe(3);
  });

-  it('should emit panel change event when toggling panels', () => {
+  it('should emit panel events when toggling panels', () => {
    const fixture = TestBed.createComponent(TestComponent);
    fixture.detectChanges();

    fixture.componentInstance.changeCallback = () => {};


please remove all these:

fuxture.componentInstance.xxx = () => {}

They're already declared in the TestComponent, no need to re-declare them

maxokorokov · 2020-06-24T09:24:25Z

src/accordion/accordion.spec.ts

+        const onHiddenSpy = spyOn(fixture.componentInstance, 'onHidden');
+
+        // First we're going to collapse, then expand
+        let opened = true;


why have this flag? it's not used anywhere

maxokorokov · 2020-06-24T09:25:25Z

src/accordion/accordion.ts

+  /**
+   * If `true`, accordion will be animated.
+   */
+  @Input() animation = false;


This is still not fixed

maxokorokov · 2020-06-24T09:33:10Z

src/accordion/accordion.ts

@@ -216,7 +239,18 @@ export class NgbAccordion implements AfterContentChecked {
   */
  @Output() panelChange = new EventEmitter<NgbPanelChangeEvent>();

-  constructor(config: NgbAccordionConfig) {
+  /**
+   * An event emitted when a panel is shown, after the transition. The payload is the panel id.


Could we be more specific, please ? meaning that "when the panel is shown" is not very clear...

Maybe An event emitted when the expanding animation is finished on the panel ?

maxokorokov · 2020-06-24T09:33:34Z

src/accordion/accordion.ts

+  @Output() shown = new EventEmitter<string>();
+
+  /**
+   * An event emitted when a panel is hidden, after the transition. The payload is the panel id.


Same here, but collapsing animation. Would also be nice to precise, if DOM content is expected to be removed at this point or not.

src/accordion/accordion.ts

fbasso · 2020-07-07T08:52:45Z

I push again with all the requested changes (I hope I didn't forget anything !).

Unfortunately, I had to change the accordion code again. This is due to some failing tests on what classes are expected (after the ngbTransition refactor with the startFn): classes managed by the transitions shouldn't be managed by bindings.

I'm not very happy with the code produced, but this is due to the initial code which would require a BIG refactor, probably in order to avoid ngAfterContentChecked usage (in favor of ngChanges for the accordion, and (headerClick) usage in panels. There are also some inconsistency (in the base code), like:

activeIds documentation : An array or comma separated strings of panel ids that should be opened initially,
should toggle panels based on "activeIds" values testing just the opposite.

I did not want to include this refactor, otherwise the review would be much harder.

maxokorokov

Hey, @fbasso, just will put a quick summary here.

In the latest version:

demos are not working
by doing _runTransitions in the afterContentCheked we're triggering it on each CD. On top of this the first thing _runTrainsitions is doing is actually detectChanges(). I think we should find a way to trigger transitions the same way we're doing for the collapse → only on binding changes, manual clicks or imperative toggles

fbasso · 2020-07-31T14:46:11Z

Last changes pushed !

I finally managed to eliminate the change detection change on AfterContentChecked, but the code required is (again) not satisfying in terms of simplicity and clarity ! The constraints given by the AfterContentChecked and the fact to not setup animation classes in the template (it produces other drawbacks and failures) is so hard that I failed to find something else 😞.

@maxokorokov , @benouat, don't hesitate to share if you have other ideas.

fbasso force-pushed the animations-accordion branch 2 times, most recently from 4505ab1 to ee3e15f Compare June 11, 2020 08:29

fbasso requested review from maxokorokov and benouat June 11, 2020 08:37

fbasso force-pushed the animations-accordion branch from ee3e15f to a3a11bf Compare June 11, 2020 12:47

maxokorokov mentioned this pull request Jun 15, 2020

Animations progress #3747

Closed

18 tasks

maxokorokov added needs: review type: animation labels Jun 17, 2020

maxokorokov added this to the 7.0.0 milestone Jun 17, 2020

maxokorokov added the type: feature label Jun 17, 2020

maxokorokov requested changes Jun 18, 2020

View reviewed changes

maxokorokov added needs: work and removed needs: review labels Jun 18, 2020

benouat requested changes Jun 18, 2020

View reviewed changes

fbasso force-pushed the animations-accordion branch 3 times, most recently from d0f666b to 45a0535 Compare June 22, 2020 14:41

maxokorokov requested changes Jun 24, 2020

View reviewed changes

fbasso force-pushed the animations-accordion branch from 45a0535 to 7683ef3 Compare July 7, 2020 08:37

maxokorokov requested changes Jul 21, 2020

View reviewed changes

fbasso force-pushed the animations-accordion branch from 7683ef3 to fe1a0fd Compare July 31, 2020 14:32

fbasso force-pushed the animations-accordion branch 2 times, most recently from 156e642 to 388b90c Compare August 3, 2020 15:30

feat(accordion): add animations

71ebf39

fbasso force-pushed the animations-accordion branch from 388b90c to 71ebf39 Compare August 5, 2020 12:20

maxokorokov removed the needs: work label Aug 5, 2020

maxokorokov merged commit 99de349 into ng-bootstrap:animations Aug 5, 2020

fbasso deleted the animations-accordion branch August 5, 2020 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(accordion): add animations #3766

feat(accordion): add animations #3766

fbasso commented Jun 11, 2020

codecov-commenter commented Jun 11, 2020 •

edited

Loading

maxokorokov left a comment •

edited

Loading

maxokorokov Jun 18, 2020

maxokorokov Jun 24, 2020

maxokorokov left a comment

maxokorokov Jun 24, 2020

fbasso Jul 6, 2020

maxokorokov Jun 24, 2020

maxokorokov Jun 24, 2020

maxokorokov Jun 24, 2020

maxokorokov Jun 24, 2020

maxokorokov Jun 24, 2020

maxokorokov Jun 24, 2020

maxokorokov Jun 24, 2020

fbasso commented Jul 7, 2020 •

edited

Loading

maxokorokov left a comment

fbasso commented Jul 31, 2020 •

edited

Loading

feat(accordion): add animations #3766

feat(accordion): add animations #3766

Conversation

fbasso commented Jun 11, 2020

codecov-commenter commented Jun 11, 2020 • edited Loading

Codecov Report

maxokorokov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxokorokov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbasso commented Jul 7, 2020 • edited Loading

maxokorokov left a comment

Choose a reason for hiding this comment

fbasso commented Jul 31, 2020 • edited Loading

codecov-commenter commented Jun 11, 2020 •

edited

Loading

maxokorokov left a comment •

edited

Loading

fbasso commented Jul 7, 2020 •

edited

Loading

fbasso commented Jul 31, 2020 •

edited

Loading