namespace the events for popover/tooltip #4104

Merged
merged 4 commits into from Jul 22, 2012

Conversation

Projects
None yet
3 participants
@lookfirst
Contributor

lookfirst commented Jul 16, 2012

namespace the events for popover/tooltip so that they can be cleanly removed.

references issue #3880

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 16, 2012

This pull request passes (merged 2ee9b27 into 40ab928).

This pull request passes (merged 2ee9b27 into 40ab928).

@lookfirst

This comment has been minimized.

Show comment
Hide comment
@lookfirst

lookfirst Jul 16, 2012

Contributor

I wasn't sure what you wanted for the config key name... ns seemed short and sweet, but i'm open to changing it however you want.

Contributor

lookfirst commented Jul 16, 2012

I wasn't sure what you wanted for the config key name... ns seemed short and sweet, but i'm open to changing it however you want.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 20, 2012

This pull request passes (merged 117f65d into 40ab928).

This pull request passes (merged 117f65d into 40ab928).

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Jul 22, 2012

Member

Hey @lookfirst,

Thanks for opening this pull-request! Unfotunately, it looks like it fails to pass the tests neccessary for submitting to bootstrap. The following tests are currently failing:

  • should always include a unit test if changing js files

For a full list of issue filing guidelines, please refer to the bootstrap issue filing guidelines.

thanks!

Member

fat commented Jul 22, 2012

Hey @lookfirst,

Thanks for opening this pull-request! Unfotunately, it looks like it fails to pass the tests neccessary for submitting to bootstrap. The following tests are currently failing:

  • should always include a unit test if changing js files

For a full list of issue filing guidelines, please refer to the bootstrap issue filing guidelines.

thanks!

@fat fat closed this Jul 22, 2012

@lookfirst

This comment has been minimized.

Show comment
Hide comment
@lookfirst

lookfirst Jul 22, 2012

Contributor

@fat umm... I did include tests.

Contributor

lookfirst commented Jul 22, 2012

@fat umm... I did include tests.

@fat fat reopened this Jul 22, 2012

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Jul 22, 2012

Member

weird, sorry about that

Member

fat commented Jul 22, 2012

weird, sorry about that

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 22, 2012

This pull request passes (merged 117f65d into 40ab928).

This pull request passes (merged 117f65d into 40ab928).

js/bootstrap-popover.js
@@ -72,7 +72,8 @@
}
, destroy: function () {
- this.$element.off().removeData('popover')
+ this.hide()
+ this.$element.off(this.options.ns).removeData('popover')

This comment has been minimized.

@lookfirst

lookfirst Jul 22, 2012

Contributor

just realized that this could be combined into this.hide().off(this.options.ns)... if you like.

@lookfirst

lookfirst Jul 22, 2012

Contributor

just realized that this could be combined into this.hide().off(this.options.ns)... if you like.

This comment has been minimized.

@fat

fat Jul 22, 2012

Member

yes pls

@fat

fat Jul 22, 2012

Member

yes pls

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Jul 22, 2012

Member

any reason not to just always use the prefix .tooltip and .popover? Not super crazy about adding an option for that

Member

fat commented Jul 22, 2012

any reason not to just always use the prefix .tooltip and .popover? Not super crazy about adding an option for that

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 22, 2012

This pull request passes (merged 393f4a7 into 40ab928).

This pull request passes (merged 393f4a7 into 40ab928).

@lookfirst

This comment has been minimized.

Show comment
Hide comment
@lookfirst

lookfirst Jul 22, 2012

Contributor

i'm not super crazy about the option either, but i could easily see a generic word like those being used by other plugins.

how about something a bit less generic like bs-popover/bs-tooltip?

Contributor

lookfirst commented Jul 22, 2012

i'm not super crazy about the option either, but i could easily see a generic word like those being used by other plugins.

how about something a bit less generic like bs-popover/bs-tooltip?

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Jul 22, 2012

Member

well... everywhere else we just use "modal" or "button". I don't think it's too big of a deal. If you have two different tooltip plugins instantiated on the same element you probably have bigger problems ;)

In the future we'll probably namespace everything with bootstrap (data-attrs and events - but that will be a 3.0.0 thing, because it's breaks backwards compatibility)

Member

fat commented Jul 22, 2012

well... everywhere else we just use "modal" or "button". I don't think it's too big of a deal. If you have two different tooltip plugins instantiated on the same element you probably have bigger problems ;)

In the future we'll probably namespace everything with bootstrap (data-attrs and events - but that will be a 3.0.0 thing, because it's breaks backwards compatibility)

@lookfirst

This comment has been minimized.

Show comment
Hide comment
@lookfirst

lookfirst Jul 22, 2012

Contributor

ok, i'll hardcode it then.

Contributor

lookfirst commented Jul 22, 2012

ok, i'll hardcode it then.

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Jul 22, 2012

Member

cool thanks :)

Member

fat commented Jul 22, 2012

cool thanks :)

@lookfirst

This comment has been minimized.

Show comment
Hide comment
@lookfirst

lookfirst Jul 22, 2012

Contributor

Oh, looking at that more closely now, I know why I did that... popover extends tooltip and tooltip.init() is where the .on() is... I needed to define the name of the plugin somewhere...

Contributor

lookfirst commented Jul 22, 2012

Oh, looking at that more closely now, I know why I did that... popover extends tooltip and tooltip.init() is where the .on() is... I needed to define the name of the plugin somewhere...

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Jul 22, 2012

Member

this.type will equal 'popover' or 'tooltip' - it's assigned in the init method

Member

fat commented Jul 22, 2012

this.type will equal 'popover' or 'tooltip' - it's assigned in the init method

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 22, 2012

This pull request passes (merged d76c899 into 40ab928).

This pull request passes (merged d76c899 into 40ab928).

@lookfirst

This comment has been minimized.

Show comment
Hide comment
@lookfirst

lookfirst Jul 22, 2012

Contributor

ah, missed that. should be good now. all tests pass.

Contributor

lookfirst commented Jul 22, 2012

ah, missed that. should be good now. all tests pass.

fat added a commit that referenced this pull request Jul 22, 2012

Merge pull request #4104 from lookfirst/2.1.0-wip-fix-destroy
namespace the events for popover/tooltip

@fat fat merged commit 48fc0ad into twbs:2.1.0-wip Jul 22, 2012

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Jul 22, 2012

Member

great - thanks man!

Member

fat commented Jul 22, 2012

great - thanks man!

@cvrebert cvrebert locked and limited conversation to collaborators Jul 28, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.