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

Add custom trigger property for $tooltip provider #289

Closed
wants to merge 1 commit into from
Closed

Add custom trigger property for $tooltip provider #289

wants to merge 1 commit into from

Conversation

L42y
Copy link
Contributor

@L42y L42y commented Apr 1, 2013

close #131

@L42y
Copy link
Contributor Author

L42y commented Apr 1, 2013

It's not complete yet, but I wonder if I'm in the right direction to implement it.

I also wonder how to implement different defaults for tooltip and popover, any idea?

@joshdmiller
Copy link
Contributor

@L42y Thanks for taking a stab at this! It's always a good idea to check in before you get too far. I'll add some inline comments to the code as I peruse.

@@ -8,11 +8,11 @@ angular.module( 'ui.bootstrap.popover', [ 'ui.bootstrap.tooltip' ] )
return {
restrict: 'EA',
replace: true,
scope: { title: '@', content: '@', placement: '@', animation: '&', isOpen: '&' },
scope: { title: '@', content: '@', placement: '@', trigger: '@', triggerHide: '@', animation: '&', isOpen: '&' },
Copy link
Contributor

Choose a reason for hiding this comment

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

These are only to give the template access to some scope variables; why does the template need to know anything about the triggers?

@joshdmiller
Copy link
Contributor

@pkozlowski-opensource The current way of operating is to assume the same trigger for closing as for opening if a closing trigger wasn't provided to the $tooltip service during directive creation. It would be sensible to extend the same functionality to the trigger attributes. But we could also have something more intelligent:

var hideTriggerDefaults = {
  'click': 'click',
  'mouseenter': 'mouseleave',
  // etc...
};

// ...

closeTrigger = closeTrigger || hideTriggerDefaults[openTrigger] || openTrigger;

@L42y Are you still working on this? I'm not rushing you; just wanting to know if I should leave this PR open. I appreciate your effort! :-)

@L42y
Copy link
Contributor Author

L42y commented Apr 5, 2013

@joshdmiller sure, still working on it. also thank you for the feedback, you're awesome :-)

@L42y
Copy link
Contributor Author

L42y commented Apr 5, 2013

I just push a new commit base on your feedback, code review round two :D
Wish I did something useful.

@pkozlowski-opensource
Copy link
Member

@L42y would be awesome if you could think of tests that exercise this new functionality. Shouldn't be difficult and we at least need a test showing that you can specify the trigger attribute.

@joshdmiller
Copy link
Contributor

@L42y Very nice! A couple of things:

  • I don't think the triggers need to be on the scope; it shouldn't matter too much, but since they are never watched and are only accessed from what's defined in the link, it should be fine to have them as regular variables. The other attributes are on the scope because they're interpolated in a string later on.
  • For the tests, we'll want a new describe block. We need to test a couple of things:
    • that triggerShow works
    • that triggerHide works
    • our logic for when triggerHide isn't provided (that's two tests because there are two possibilities)

We don't need to test when neither are provided because the entire rest of the test suite depends on the defaults working properly.

@L42y Again - very groovy, my friend.

@L42y
Copy link
Contributor Author

L42y commented Apr 6, 2013

I pushed another commit. My previous commits actually broke many things, sorry for that, i didn't run test before i push them.

This new commit still broke the custom placement property, but I can't figure out the reason, wish someone would help me, thank you.

@L42y
Copy link
Contributor Author

L42y commented Apr 12, 2013

Closing this PR due to no proper implementation, thank you so much for your kind helps, looking forward what else I can do for this awesome project.

@L42y L42y closed this Apr 12, 2013
codedogfish pushed a commit to codedogfish/angular-ui-bootstrap that referenced this pull request Sep 15, 2015
codedogfish pushed a commit to codedogfish/angular-ui-bootstrap that referenced this pull request Sep 15, 2015
codedogfish pushed a commit to codedogfish/angular-ui-bootstrap that referenced this pull request Sep 15, 2015
angular-ui#289 - Multiple select to not set form to dirty as soon as it load
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider add trigger property to popover
3 participants