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(example-app): Sidenav closes with Esc #1244

Merged
merged 4 commits into from
Aug 16, 2018
Merged

Conversation

mgred
Copy link
Contributor

@mgred mgred commented Aug 12, 2018

See: #1172

mgred added 2 commits August 12, 2018 13:18
* This dispatches `CloseSidenav` on closedStart event

IMPORTANT: This does not work properly!
After unfocusing the sidenav, the element cannot be closed with
`escape` again, at least on the login screen. On other screens
it's not working at all.
* (keydown.escape) explicitly trigers `close` on mat-sidenav

This fixes the behaviour that the sidenav cannot be closed with
`escape` after unfocusing.
@coveralls
Copy link

coveralls commented Aug 12, 2018

Coverage Status

Coverage remained the same at 88.618% when pulling b938f5d on mgred:ngrx/1172 into 0c26a95 on ngrx:master.

@mgred mgred mentioned this pull request Aug 12, 2018
@mgred mgred changed the title fix(example-app): Sidenav closes with Esc - fix(example-app): Sidenav closes with Esc Aug 12, 2018
constructor(private store: Store<fromRoot.State>) {}

closeSidenav() {
this.store.dispatch(new LayoutActions.CloseSidenav());
Copy link
Member

Choose a reason for hiding this comment

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

Following the dump/smart component convention we can't dispatch inside this pure component,
we have to use an @Output property to emit the close event to the container page located at example-app/app/core/containers/app.component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @timdeschryver, I will change this according to your suggestions.

dump/smart component convention, now dispatching the closing
action in the containter component (app.component)
See: ngrx#1172

@Component({
selector: 'bc-sidenav',
template: `
<mat-sidenav [opened]="open">
<mat-sidenav #sidenav [opened]="open" (keydown.escape)="sidenav.close()" (closedStart)="closeMenu.emit()" disabelClose>
Copy link
Member

Choose a reason for hiding this comment

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

disabelClose -> disableClose

Copy link
Member

Choose a reason for hiding this comment

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

Why not just call closeSidenav.emit() on both events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandonroberts,

disabelClose -> disableClose

fixed this typo

Why not just call closeSidenav.emit() on both events?

I think this does not work, because we have to explicitly call the "closing" event of the menu when escape was pressed on the focused element. Otherwise it would never close since disableClose is set.

@@ -19,4 +19,5 @@ import { Component, Input } from '@angular/core';
})
export class SidenavComponent {
@Input() open = false;
@Output() closeMenu = new EventEmitter();
Copy link
Member

Choose a reason for hiding this comment

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

closeMenu -> closeSidenav

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandonroberts, I named this according to the "opening" event defined in toolbar.component which is called openMenu.

In app.component:

<bc-toolbar (openMenu)="openSidenav()">
...
</bc-toolbar>

@brandonroberts brandonroberts merged commit b3fc5dd into ngrx:master Aug 16, 2018
@brandonroberts
Copy link
Member

Thanks @mgred!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants