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

fix(mdToolbar): Allow md-scroll-shrink usage with ng-if. #4394

Closed
wants to merge 1 commit into from

Conversation

topherfangio
Copy link
Contributor

Previously, the toolbar would fail to enable scroll shrinking
if the developer used an ng-if statement on the md-toolbar.

This commit allows usage of ng-if as well as watching the
md-scroll-shrink for changes so they can enable/disable
scroll shrinking.

Fixes #2751.

@topherfangio topherfangio added the needs: review This PR is waiting on review from the team label Aug 31, 2015
@topherfangio topherfangio added this to the 0.12.0 milestone Aug 31, 2015
@topherfangio
Copy link
Contributor Author

@ThomasBurleson When you review this PR, could you please check to ensure this doesn't constitute a breaking change?

Particularly, I've added an isolate scope, but it should be okay since I added the transclusion. I just wanted a second pair of eyes.

@@ -113,7 +157,7 @@ function mdToolbarDirective($$rAF, $mdConstant, $mdUtil, $mdTheming, $animate )
//
// As the user scrolls down, the content will be transformed up slowly
// to put the content underneath where the toolbar was.
var margin = (-toolbarHeight * shrinkSpeedFactor) + 'px';
var margin = (-toolbarHeight * shrinkSpeedFactor) + 'px';
contentElement.css('margin-top', margin);
contentElement.css('margin-bottom', margin);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is better:

contentElement.css({
 'margin-top' : margin,
 'margin-bottom' : margin
});

@ThomasBurleson ThomasBurleson added needs: work and removed needs: review This PR is waiting on review from the team labels Sep 1, 2015

$$rAF(updateToolbarHeight);
}

function onMdContentLoad($event, newContentEl) {
// Toolbar and content must be siblings
if (element.parent()[0] === newContentEl.parent()[0]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI - scope.$on(...) will call onMdContentLoad( ) without the 2nd argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In what case will it do so? All of my testing showed that it always passed the newContentEl.

Previously, the toolbar would fail to enable scroll shrinking
if the developer used an `ng-if` statement on the `md-toolbar`.

This commit allows usage of `ng-if` as well as watching the
`md-scroll-shrink` for changes so they can enable/disable
scroll shrinking.

Fixes #2751.
@topherfangio topherfangio force-pushed the fix-toolbar-scroll-shrink-if-2751 branch from 9e5432e to 2ec70ea Compare September 1, 2015 15:47
@topherfangio topherfangio deleted the fix-toolbar-scroll-shrink-if-2751 branch September 10, 2015 20:41
kennethcachia pushed a commit to kennethcachia/material that referenced this pull request Sep 23, 2015
Previously, the toolbar would fail to enable scroll shrinking
if the developer used an `ng-if` statement on the `md-toolbar`.

This commit allows usage of `ng-if` as well as watching the
`md-scroll-shrink` for changes so they can enable/disable
scroll shrinking.

Fixes angular#2751. Closes angular#4394.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants