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

fix(sidenav): allow for data bindings in md-component-id #9255

Merged
merged 1 commit into from
Sep 21, 2016
Merged

fix(sidenav): allow for data bindings in md-component-id #9255

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
24 changes: 19 additions & 5 deletions src/components/sidenav/sidenav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,7 @@ function SidenavDirective($mdMedia, $mdUtil, $mdConstant, $mdTheming, $animate,
},
controller: '$mdSidenavController',
compile: function(element) {
element.addClass('md-closed');
element.attr('tabIndex', '-1');
element.addClass('md-closed').attr('tabIndex', '-1');
return postLink;
}
};
Expand Down Expand Up @@ -481,9 +480,8 @@ function SidenavDirective($mdMedia, $mdUtil, $mdConstant, $mdTheming, $animate,
* @ngdoc controller
* @name SidenavController
* @module material.components.sidenav
*
*/
function SidenavController($scope, $element, $attrs, $mdComponentRegistry, $q) {
function SidenavController($scope, $attrs, $mdComponentRegistry, $q, $interpolate) {

var self = this;

Expand All @@ -505,5 +503,21 @@ function SidenavController($scope, $element, $attrs, $mdComponentRegistry, $q) {
self.toggle = function() { return self.$toggleOpen( !$scope.isOpen ); };
self.$toggleOpen = function(value) { return $q.when($scope.isOpen = value); };

self.destroy = $mdComponentRegistry.register(self, $attrs.mdComponentId);
// Evaluate the component id.
var rawId = $attrs.mdComponentId;
var hasDataBinding = rawId && rawId.indexOf($interpolate.startSymbol()) > -1;
Copy link
Member

@EladBezalel EladBezalel Sep 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Is mdComponentId a directive?
If so why isn't that check happens on the directive? if not let's make it a directive.. looks like it should be

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 a directive, it's just the attribute that you use to pass the id. I'm not sure it make sense for it to be a directive since it's just a string.

Copy link
Member

Choose a reason for hiding this comment

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

It can be an attribute directive that checks if it should watch for changes or not (like mdColors does)

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 still seems redundant and won't be immefiately obvious where the id is coming from. In this case it's right next to the initialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

As per the discussion on Slack, let's keep this in here for now and do a more generic solution if the need for one comes up.

var componentId = hasDataBinding ? $interpolate(rawId)($scope.$parent) : rawId;

// Register the component.
self.destroy = $mdComponentRegistry.register(self, componentId);

// Watch and update the component, if the id has changed.
if (hasDataBinding) {
$attrs.$observe('mdComponentId', function(id) {
if (id && id !== self.$$mdHandle) {
Copy link
Member

@EladBezalel EladBezalel Sep 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

what's $$mdHandle?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the id that is assigned when an element is registered with the $mdComponentRegistry. (https://github.com/angular/material/blob/master/src/core/services/registry/componentRegistry.js#L64)

Copy link
Contributor

@ThomasBurleson ThomasBurleson Sep 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

why not call it self.componentId ? Use of $$mdHandle seems very wrong here.

Copy link
Member Author

Choose a reason for hiding this comment

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

self.componentId isn't actually being declared, it's only in the $attrs, $$mdHandle is the one that keeps track of the current id.

self.destroy(); // `destroy` only deregisters the old component id so we can add the new one.
self.destroy = $mdComponentRegistry.register(self, id);
}
});
}
}
48 changes: 30 additions & 18 deletions src/components/sidenav/sidenav.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
describe('mdSidenav', function() {
beforeEach(module('material.components.sidenav'));

function setup(attrs) {
function setup(attrs, skipInitialDigest) {
var el;
inject(function($compile, $rootScope) {
var parent = angular.element('<div>');
el = angular.element('<md-sidenav ' + (attrs || '') + '>');
parent.append(el);
$compile(parent)($rootScope);
$rootScope.$apply();
!skipInitialDigest && $rootScope.$apply();
});
return el;
}
Expand Down Expand Up @@ -168,13 +168,13 @@ describe('mdSidenav', function() {

describe('controller', function() {
it('should create controller', function() {
var el = setup('');
var el = setup();
var controller = el.controller('mdSidenav');
expect(controller).not.toBe(undefined);
});

it('should open and close and toggle', inject(function($timeout) {
var el = setup('');
var el = setup();
var scope = el.isolateScope();
var controller = el.controller('mdSidenav');

Expand Down Expand Up @@ -213,7 +213,7 @@ describe('mdSidenav', function() {
}));

it('should open(), close(), and toggle() with promises', function() {
var el = setup('');
var el = setup();
var scope = el.isolateScope();
var controller = el.controller('mdSidenav');

Expand Down Expand Up @@ -257,7 +257,7 @@ describe('mdSidenav', function() {
});

it('should open() to work multiple times before close()', function() {
var el = setup('');
var el = setup();
var controller = el.controller('mdSidenav');

var openDone = 0, closeDone = 0;
Expand Down Expand Up @@ -351,23 +351,35 @@ describe('mdSidenav', function() {
});

describe('$mdSidenav lookups', function() {
var $rootScope, $timeout;
var $rootScope, $timeout, $mdSidenav;

beforeEach(inject(function(_$rootScope_, _$timeout_) {
beforeEach(inject(function(_$rootScope_, _$timeout_, _$mdSidenav_) {
$rootScope = _$rootScope_;
$timeout = _$timeout_;
$mdSidenav = _$mdSidenav_;
}));

it('should find an instantiation using `$mdSidenav(id)`', inject(function($mdSidenav) {
it('should find an instantiation using `$mdSidenav(id)`', function() {
var el = setup('md-component-id="left"');
$timeout.flush();

// Lookup instance still available in the component registry
var instance = $mdSidenav('left');
expect(instance).toBeTruthy();
}));
});

it('should find a deferred instantiation using `$mdSidenav(id, true)`', inject(function($mdSidenav) {
it('should support data bindings', function() {
// It should work on init.
$rootScope.leftComponentId = 'left';
setup('md-component-id="{{ leftComponentId }}"', true);
expect($mdSidenav($rootScope.leftComponentId, false)).toBeTruthy();

// It should also work if the data binding has changed.
$rootScope.$apply('leftComponentId = "otherLeft"');
expect($mdSidenav($rootScope.leftComponentId, false)).toBeTruthy();
});

it('should find a deferred instantiation using `$mdSidenav(id, true)`', function() {
var instance;

// Lookup deferred (not existing) instance
Expand All @@ -386,9 +398,9 @@ describe('mdSidenav', function() {
// Lookup instance still available in the component registry
instance = $mdSidenav('left', true);
expect(instance).toBeTruthy();
}));
});

it('should find a deferred instantiation using `$mdSidenav().waitFor(id)` ', inject(function($mdSidenav) {
it('should find a deferred instantiation using `$mdSidenav().waitFor(id)` ', function() {
var instance;

// Lookup deferred (not existing) instance
Expand All @@ -409,9 +421,9 @@ describe('mdSidenav', function() {
instance = $mdSidenav('left');

expect(instance).toBeTruthy();
}));
});

it('should not find a lazy instantiation without waiting `$mdSidenav(id)`', inject(function($mdSidenav) {
it('should not find a lazy instantiation without waiting `$mdSidenav(id)`', function() {
var instance = $mdSidenav('left');
expect(instance.isOpen).toBeDefined(); // returns legacy API with noops

Expand All @@ -425,9 +437,9 @@ describe('mdSidenav', function() {
instance = $mdSidenav('left'); // returns instance
expect(instance).toBeDefined();
expect(instance.isOpen()).toBeFalsy();
}));
});

it('should not find a lazy instantiation without waiting `$mdSidenav().find(id)`', inject(function($mdSidenav) {
it('should not find a lazy instantiation without waiting `$mdSidenav().find(id)`', function() {
var instance = $mdSidenav().find('left');
expect(instance).toBeUndefined();

Expand All @@ -438,7 +450,7 @@ describe('mdSidenav', function() {
instance = $mdSidenav().find('left');
expect(instance).toBeDefined();
expect(instance.isOpen()).toBeFalsy();
}));
});

describe('onClose', function () {
it('should call callback on escape', inject(function($mdSidenav, $rootScope, $material, $mdConstant, $timeout) {
Expand Down