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

feat(expansion): emit animation events for expansion panels #12412

Merged
merged 1 commit into from
Aug 16, 2018
Merged

feat(expansion): emit animation events for expansion panels #12412

merged 1 commit into from
Aug 16, 2018

Conversation

josephperrott
Copy link
Member

Fixes #11838

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 27, 2018
@josephperrott josephperrott changed the title feat(expansion): add animation events for expansion panels feat(expansion): emit animation events for expansion panels Jul 27, 2018
crisbeto
crisbeto reviewed Jul 27, 2018
View reviewed changes
src/lib/expansion/expansion-panel.ts Outdated
@@ -78,6 +80,18 @@ export class MatExpansionPanel extends CdkAccordionItem
}
private _hideToggle = false;

/** An event emitted before the body's expansion animation happens. */
@Output() beforeExpanded = new EventEmitter<null>();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether emitting null is particularly useful. We should either switch it to void or emit the AnimationEvent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely meant void and then typed null

src/lib/expansion/expansion-panel.ts Outdated
if (fromState !== 'void') {
this.afterExpanded.emit();
}
return;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether all of these returns are necessary. If the state is something different, it just won't go into the other conditions. Also can we break this up a bit since it's a fair bit of if-s in one place?

src/lib/expansion/expansion.spec.ts Outdated
const afterExpanded = fixture.componentInstance.panel.afterExpanded;
const beforeCollapsed = fixture.componentInstance.panel.beforeCollapsed;
const afterCollapsed = fixture.componentInstance.panel.afterCollapsed;
spyOn(beforeExpanded, 'emit');
Copy link
Member

Choose a reason for hiding this comment

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

We should subscribe to the EventEmitter, rather than stubbing out its emit method.

jelbourn
jelbourn reviewed Jul 27, 2018
View reviewed changes
src/demo-app/expansion/expansion-demo.html Outdated
@@ -1,6 +1,10 @@
<h1>Single Expansion Panel</h1>

<mat-expansion-panel class="demo-expansion-width" #myPanel>
<mat-expansion-panel class="demo-expansion-width" #myPanel
(beforeExpanded)="addEvent('beforeExpanded')"
Copy link
Member

Choose a reason for hiding this comment

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

Indent +4?

src/lib/expansion/expansion-panel.ts Outdated
_changeDetectorRef: ChangeDetectorRef,
_uniqueSelectionDispatcher: UniqueSelectionDispatcher,
private _viewContainerRef: ViewContainerRef) {
_changeDetectorRef: ChangeDetectorRef,
Copy link
Member

Choose a reason for hiding this comment

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

Indent should be +4

src/lib/expansion/expansion-panel.ts Outdated
@Output() beforeCollapsed = new EventEmitter<null>();

/** An event emitted after the body's collapse animation happens. */
@Output() afterCollapsed = new EventEmitter<null>();
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on e.g. afterExpand vs. afterExpanded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to match the other usages in the library

@josephperrott josephperrott added the target: minor This PR is targeted for the next minor release label Jul 27, 2018
@josephperrott josephperrott force-pushed the expansion-panel-events branch 4 times, most recently from 98a8cf1 to 699283f Compare July 30, 2018 16:34
@josephperrott josephperrott mentioned this pull request Aug 2, 2018
Closed
crisbeto
crisbeto approved these changes Aug 13, 2018
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.

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 13, 2018
@mmalerba mmalerba merged commit f6b1002 into angular:master Aug 16, 2018
@Swoox Swoox mentioned this pull request Sep 27, 2018
Closed
@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 9, 2019
@josephperrott josephperrott deleted the expansion-panel-events branch March 20, 2020 22:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@crisbeto crisbeto crisbeto approved these changes

Assignees

@crisbeto crisbeto

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expansion Panel panel-content on animation end event
5 participants
@josephperrott @jelbourn @crisbeto @googlebot @mmalerba