fix(sidenav): correctly detect when sidenav align changes. #1758
Conversation
googlebot
added
the
cla: yes
mmalerba
changed the title
| value = (value == 'end') ? 'end' : 'start'; | ||
| if (value != this._align) { | ||
| this._align = value; | ||
| this.onAlignChanged.emit(); |
There was a problem hiding this comment.
Why not just directly call validateDrawers here instead of going through an observable?
There was a problem hiding this comment.
This code is part of MdSidenav, validateDrawers is part of MdSidenavLayout, so I couldn't call it directly.
| if (!value) { | ||
| this.close(); | ||
| } | ||
| this._valid = value; |
There was a problem hiding this comment.
I'd do this as
this._valid = coerceBooleanProperty(value);
if (!this._valid) {
this.close();
}I generally like to avoid overwriting function arguments.
Can you also add a comment explaining why you want to close the sidenav in this case?
There was a problem hiding this comment.
Added a comment, but I did it this way because once _valid is false toggle (called by close) just returns without doing anything.
| @@ -275,48 +313,74 @@ export class MdSidenavLayout implements AfterContentInit { | |||
| } | |||
| } | |||
|
|
|||
| /** TODO: internal */ | |||
| /** @override */ | |||
There was a problem hiding this comment.
I don't think the @override is necessary
There was a problem hiding this comment.
Is there something else we can put here to indicate that it's being implemented as part of an interface? The rest of the TODO: internal's we prefixed with an underscore, but we can't do that here.
There was a problem hiding this comment.
For lifecycle events it's fine to just leave it- people should just know not to call them.
|
Can you also update the README to mention this invalid state? |
|
Done |
jelbourn
added
pr: lgtm
action: merge
|
Hi Angular Team, I have used sidenav component for displaying menu. When I click on the menu item it won't disappear but redirect me to that page and menu is still there. I have used all three modes (side, push, over). |
|
@gatuteck You can file bug reports here: https://github.com/angular/material2/issues. Be sure to follow the issue template. Commenting on random pull requests doesn't accomplish anything. |
mmalerba
changed the title
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #1236