Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Tooltip :: Enable ngModel to use parent scope #2151

Closed
wants to merge 5 commits into from

Conversation

mattslocum
Copy link

Tooltip used to use a child scope. So elements that use ng-model with primitives don't update the parent scope. http://jsfiddle.net/mslocum/CEqA8/

Solution:
This makes ngModel still have 2-way data binding with elements. http://jsfiddle.net/mslocum/CEqA8/2

mattslocum and others added 3 commits May 5, 2014 22:59
Tooltip used to use a child scope. http://jsfiddle.net/mslocum/CEqA8/

This makes ngModel still have 2-way data binding to elements. http://jsfiddle.net/mslocum/CEqA8/1
Child scope prevented tooltips on input elements from binding
to the correct scope.

Tests have been updated accordingly.

Fixes angular-ui#775
Fixes angular-ui#2092
@mattslocum
Copy link
Author

I'm aware that currently a workaround would be to either bind the model with an object or to use $parent, but neither of these are intuitive. Adding a 'tooltip' attribute should not break normal input functionality.

@joshdmiller
Copy link
Contributor

We opted to use a child scope because an isolate scope would cause problems with any binding or expression used within the text of the tooltip rather than just with ngModel. The best solution in your case is to bind to an object property instead, I would think.

@mattslocum
Copy link
Author

But I am able to still run expressions and functions in the tooltip when there is isolate scope.

http://jsfiddle.net/mslocum/CEqA8/5/

Matt Slocum added 2 commits May 7, 2014 17:41
Child scope prevented tooltips on input elements from binding
to the correct scope.

Tests have been updated accordingly.

Fixes angular-ui#775
Fixes angular-ui#2092
@mattslocum
Copy link
Author

@joshdmiller Check out my changes now.

You were using attrs.$observe on all attrs except Enable and Animate, so this was getting around the isolate scope except for those. I updated Enable and Animate to also use $observe. From what I can tell, all functionality is now identical to the child scope code, but now it doesn't break things like ng-model. In my mind, adding a tooltip attribute shouldn't break existing functionality of inputs.

http://jsfiddle.net/mslocum/CEqA8/7/

I wanted to do

scope: {
    tt_enable: '=' + prefix + 'Enable',
    tt_animation: '=' + prefix + 'Animation'
},

but then the html binding would look like:

<input type="checkbox" 
    tooltip="This is a tooltip" 
    tooltip-enable="bEnable"
/>

I think this would have been a breaking change as all the test specs do: tooltip-enable="{{bEnable}}", so I enable and animation will always require {{ }} in my current solution.

@chrisirhc
Copy link
Contributor

@mattslocum There's some previous work on this on #1600 and #1848. I think we didn't reach a consensus yet on the refactored design of the tooltip, hence this issue has been stuck for a while.

@mattslocum
Copy link
Author

I didn't see those before I did my PR. #1848 looks really cool! If a consensus is not reached before the next stable release, I would vote on getting a more basic isolate scope solution out because of how child scope is breaking ng-model.

@wesleycho
Copy link
Contributor

@chrisirhc any thoughts on this now that the tooltip refactor is done?

@mattslocum
Copy link
Author

It looks like #1848 just got merged 9 days ago. I'll have to give it a try, but I doubt my PR is needed anymore.

@chrisirhc
Copy link
Contributor

Yep, this shouldn't be required anymore. The refactor has removed the scope definition on the tooltip directive. @mattslocum Thanks for your work here. Feel free to open an issue if you find that this isn't resolved, as it's definitely a bug.

@chrisirhc chrisirhc closed this Apr 6, 2015
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.

5 participants