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

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Aug 6, 2016

Fixes the sidenav not evaluating expressions inside of the md-component-id attribute.

Fixes #9052.

@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label Aug 6, 2016
@devversion
Copy link
Member

I think @EladBezalel had something similar, including observation of the attribute. #5972

@crisbeto
Copy link
Member Author

crisbeto commented Aug 6, 2016

Seems like it didn't end up getting merged. @EladBezalel is it ok if I incorporate the changes here?

@crisbeto crisbeto force-pushed the 9052/sidenav-expression branch 2 times, most recently from 053d671 to 4cc21fe Compare August 13, 2016 10:39
@crisbeto
Copy link
Member Author

crisbeto commented Aug 13, 2016

Updated, although the approach from #5972 doesn't work for cases where the instance is being looked up in a controller on load (like in the sidenav demo).

self.destroy = $mdComponentRegistry.register(self, componentId);

// Watch and update the component, if the id has changed.
$attrs.$observe('mdComponentId', function(id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we only do this if we originally had interpolation symbols ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yeah, good point. I'll change it.

Fixes the sidenav not evaluating expressions inside of the `md-component-id` attribute.

Fixes angular#9052.
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

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.

@ThomasBurleson ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: presubmit and removed needs: review This PR is waiting on review from the team in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Sep 9, 2016
@jelbourn jelbourn merged commit 5cdceeb into angular:master Sep 21, 2016
Frank3K pushed a commit to Frank3K/material that referenced this pull request Oct 8, 2016
Fixes the sidenav not evaluating expressions inside of the `md-component-id` attribute.

Fixes angular#9052.
@Splaktar Splaktar added this to the 1.1.2 milestone 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants