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

fix(toolbar): add support in scrollshrink to ngshow/hide #5863

Closed
wants to merge 1 commit into from
Closed

fix(toolbar): add support in scrollshrink to ngshow/hide #5863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/components/toolbar/toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ function mdToolbarDirective($$rAF, $mdConstant, $mdUtil, $mdTheming, $animate) {

attr.$observe('mdScrollShrink', onChangeScrollShrink);

// If the toolbar has ngShow or ngHide we need to update height immediately as it changed
// and not wait for $mdUtil.debounce to happen

if (attr.ngShow) { scope.$watch(attr.ngShow, updateToolbarHeight); }
Copy link
Contributor

Choose a reason for hiding this comment

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

@EladBezalel - why use scope.$watch() instead of attr.$observer()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially it was $observe but than topher came with this jem:
a quick excerpt from here regarding $observe verses $watch and why we need $watch here: http://stackoverflow.com/questions/14876112/angularjs-difference-between-the-observe-and-watch-methods

(If you try attrs.$observe('attr1') you'll get the string myModel.some_prop, which is probably not what you want.)

The $observe was returning the string ‘shouldShowToolbar’, not the value of that expression. And it was only firing once because the expression itself didn’t change.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a note, I'm pretty sure the attr.$obseve('mdScrollShrink', onChangeScrollShrink); line above is also broken in the same way but I didn't get a chance to test.

Also, I highly doubt that people will be dynamically changing the scrollShrink attribute of the toolbar, so it's a VERY low priority to fix :-)

if (attr.ngHide) { scope.$watch(attr.ngHide, updateToolbarHeight); }

// If the scope is destroyed (which could happen with ng-if), make sure
// to disable scroll shrinking again

Expand Down
104 changes: 104 additions & 0 deletions src/components/toolbar/toolbar.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,110 @@ describe('<md-toolbar>', function() {
expect(element.find('md-toolbar').length).toBe(0);
}));

it('works with ng-show', inject(function($$rAF) {
var template =
'<div layout="column" style="height: 600px;">' +
' <md-toolbar md-scroll-shrink="true" ng-show="shouldShowToolbar">test</md-toolbar>' +
' <md-content flex><div style="height: 5000px;"></div></md-content>' +
'</div>';

// Build/append the element
build(template);
document.body.appendChild(element[0]);

// Flushing to get the actual height of toolbar
$$rAF.flush();

//
// Initial tests
//

var toolbarStyles = getComputedStyle(element.find('md-toolbar')[0]);
var contentStyles = getComputedStyle(element.find('md-content')[0]);

// Should start out hidden because we have not set shouldShowToolbar
expect(toolbarStyles.display).toBeTruthy();
expect(toolbarStyles.display).toEqual('none');

// Expect the content to have a zero margin top
expect(contentStyles.marginTop).toBeTruthy();
expect(contentStyles.marginTop).toEqual('0px');

//
// After showing toolbar tests
//

// Show the toolbar and ensure it is visible
pageScope.$apply('shouldShowToolbar = true');
pageScope.$digest();

toolbarStyles = getComputedStyle(element.find('md-toolbar')[0]);
contentStyles = getComputedStyle(element.find('md-content')[0]);

// Expect the toolbar to be visible
expect(toolbarStyles.display).toBeTruthy();
expect(toolbarStyles.display).not.toEqual('none');

// Expect the content to have a non-zero margin top (because updateToolbarHeight() was called)
expect(contentStyles.marginTop).toBeTruthy();
expect(contentStyles.marginTop).not.toEqual('0px');

// Remove the element
document.body.removeChild(element[0]);
}));

it('works with ng-hide', inject(function($$rAF) {
var template =
'<div layout="column" style="height: 600px;">' +
' <md-toolbar md-scroll-shrink="true" ng-hide="shouldNotShowToolbar">test</md-toolbar>' +
' <md-content flex><div style="height: 5000px;"></div></md-content>' +
'</div>';

// Build/append the element
build(template);
document.body.appendChild(element[0]);

// Flushing to get the actual height of toolbar
$$rAF.flush();

//
// Initial tests
//

var toolbarStyles = getComputedStyle(element.find('md-toolbar')[0]);
var contentStyles = getComputedStyle(element.find('md-content')[0]);

// Should start out visible because we have not set shouldNotShowToolbar
expect(toolbarStyles.display).toBeTruthy();
expect(toolbarStyles.display).not.toEqual('none');

// Expect the content to have a non-zero margin top
expect(contentStyles.marginTop).toBeTruthy();
expect(contentStyles.marginTop).not.toEqual('0px');

//
// After showing toolbar tests
//

// Show the toolbar and ensure it is hidden
pageScope.$apply('shouldNotShowToolbar = true');
pageScope.$digest();

toolbarStyles = getComputedStyle(element.find('md-toolbar')[0]);
contentStyles = getComputedStyle(element.find('md-content')[0]);

// Expect the toolbar to be hidden
expect(toolbarStyles.display).toBeTruthy();
expect(toolbarStyles.display).toEqual('none');

// Expect the content to have a zero margin top (because updateToolbarHeight() was called)
expect(contentStyles.marginTop).toBeTruthy();
expect(contentStyles.marginTop).toEqual('0px');

// Remove the element
document.body.removeChild(element[0]);
}));

// The toolbar is like a container component, so we want to make sure it works with ng-controller
it('works with ng-controller', inject(function($exceptionHandler) {
build(
Expand Down