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

feat(showHide): Introduce directives to notify child directives on hi… #5579

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions src/components/showHide/showHide.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* @ngdoc module
* @name material.components.showHide
*/

// Add additional handlers to ng-show and ng-hide that notify directives
// contained within that they should recompute their size.
// These run in addition to Angular's built-in ng-hide and ng-show directives.
angular.module('material.components.showHide', [
'material.core'
])
.directive('ngShow', createDirective('ngShow', true))
Copy link
Member

Choose a reason for hiding this comment

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

I would move part of the later comment here, saying something like

// Create our own directives 'ngShow' and 'ngHide' that are run in 
// _addition_ to the normal ngShow/ngHide. We use these to...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

.directive('ngHide', createDirective('ngHide', false));


function createDirective(name, targetValue) {
return ['$mdUtil', function($mdUtil) {
return {
restrict: 'A',
multiElement: true,
link: function($scope, $element, $attr) {
$scope.$watch($attr[name], function(value) {
if (!!value === targetValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

$mdUtil.nextTick(function() {
$scope.$broadcast('$md-resize');
});
$mdUtil.dom.animator.waitTransitionEnd($element).then(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kseamon - why are you broadcasting 2x?

Is this to support ng-hide/ng-show changes that do not have associated animations ?. FYI - waitTransition( ) has a built-in timeout of 3secs which will cause another $md-resize event to be broadcast. Is this the desired effect ?

@jelbourn - your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my concern was that not every element has transitions, and 3s is longer than I'd like to wait in those cases.

$scope.$broadcast('$md-resize');
});
}
});
}
};
}];
}
86 changes: 86 additions & 0 deletions src/components/showHide/showHide.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
describe('showHide', function() {
var $compile, $timeout, defered, scope, spy;

beforeEach(module('material.components.showHide'));

beforeEach(inject(function(_$compile_, $mdUtil, $q, $rootScope, _$timeout_) {
$compile = _$compile_;
$timeout = _$timeout_;
defered = $q.defer();
scope = $rootScope.$new();
spy = jasmine.createSpy();

scope.$on('$md-resize', spy);
spyOn($mdUtil.dom.animator, 'waitTransitionEnd').and.returnValue(defered.promise);
}));

afterEach(function() {
scope.$destroy();
});

describe('ng-hide', function() {
it('should notify when the node unhides', function() {
scope.hide = true;
var element = $compile('<div ng-hide="hide"></div>')(scope);
scope.$apply();
expect(spy).not.toHaveBeenCalled();

// Expect a $broadcast when showing.
scope.hide = false;
scope.$apply();
$timeout.flush();
expect(spy).toHaveBeenCalled();

// Expect a $broadcast on transitionEnd after showing.
spy.calls.reset();
defered.resolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

What are you doing here... it is not clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments

scope.$apply();
expect(spy).toHaveBeenCalled();
});

it('should not notify on hide', function() {
scope.hide = true;
var element = $compile('<div ng-hide="hide"></div>')(scope);
scope.$apply();

// Expect no $broadcasts when hiding.
expect(spy).not.toHaveBeenCalled();
defered.resolve();
scope.$apply();
expect(spy).not.toHaveBeenCalled();
});
});

describe('ng-show', function() {
it('should notify when the node unhides', function() {
scope.show = false;
var element = $compile('<div ng-show="show"></div>')(scope);
scope.$apply();
expect(spy).not.toHaveBeenCalled();

// Expect a $broadcast when showing.
scope.show = true;
scope.$apply();
$timeout.flush();
expect(spy).toHaveBeenCalled();

// Expect a $broadcast on transitionEnd after showing.
spy.calls.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you break this group out into a understand function call ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the comments added are sufficient. Have a look.

defered.resolve();
scope.$apply();
expect(spy).toHaveBeenCalled();
});

it('should not notify on hide', function() {
scope.show = false;
var element = $compile('<div ng-show="show"></div>')(scope);
scope.$apply();

// Expect no $broadcasts when hiding.
expect(spy).not.toHaveBeenCalled();
defered.resolve();
scope.$apply();
expect(spy).not.toHaveBeenCalled();
});
});
});