Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(sidenav): notify child components when the element is opened #9512

Merged
merged 1 commit into from
Sep 9, 2016

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Sep 3, 2016

Triggers a resize event whenever a sidenav is opened. This allows components like virtualRepeat and textarea to resize properly.

Fixes #7309.

@topherfangio you and @devversion had a discussion on the issue about having a more generic solution to this. What do you think about having the changes from here until that happens?

Triggers a resize event whenever a sidenav is opened. This allows components like virtualRepeat and textarea to resize properly.

Fixes angular#7309.
@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label Sep 3, 2016
@crisbeto crisbeto added this to the 1.1.2 milestone Sep 3, 2016
@crisbeto crisbeto added the needs: team discussion This issue requires a decision from the team before moving forward. label Sep 3, 2016
@devversion
Copy link
Member

devversion commented Sep 3, 2016

I think this is good for now. But I would not close that issue as for this PR (we could also file a new issue and assign it to Topher)

@topherfangio
Copy link
Contributor

LGTM 👍

@crisbeto crisbeto removed the needs: team discussion This issue requires a decision from the team before moving forward. label Sep 3, 2016
@@ -365,6 +367,8 @@ function SidenavDirective($mdMedia, $mdUtil, $mdConstant, $mdTheming, $animate,
]).then(function() {
// Perform focus when animations are ALL done...
if (scope.isOpen) {
// Notifies child components that the sidenav was opened.
ngWindow.triggerHandler('resize');
Copy link
Contributor

Choose a reason for hiding this comment

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

So after the SideNav open animation finishes, you fire a resize event that causes reflows for the entire window?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not really a resize event, it just fires all of the resize handlers. An alternative to this can be to fire off a $md-resize event which is only for the virtual repeaters. I decided to go with this so other components could take advantage of it.

Copy link
Contributor

@ThomasBurleson ThomasBurleson Sep 3, 2016

Choose a reason for hiding this comment

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

The handlers have to be, however, attached to the window right ? So no notification of window children. What about bubbling from the sideNav parent up?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, however we can assume that all the handlers are registered on the window, because the "resize" event doesn't work on anything else.

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Sep 7, 2016
@kara kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Sep 9, 2016
@kara kara merged commit 989f81e into angular:master Sep 9, 2016
Frank3K pushed a commit to Frank3K/material that referenced this pull request Sep 11, 2016
…ular#9512)

Triggers a resize event whenever a sidenav is opened. This allows components like virtualRepeat and textarea to resize properly.

Fixes angular#7309.
@Splaktar Splaktar added the P4: minor Minor issues. May not be fixed without community contributions. label Mar 28, 2019
@angular angular locked as resolved and limited conversation to collaborators Mar 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P4: minor Minor issues. May not be fixed without community contributions. pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants