Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tooltip self-reference to address #12320 #13593

Merged
merged 1 commit into from
May 23, 2014
Merged

Add tooltip self-reference to address #12320 #13593

merged 1 commit into from
May 23, 2014

Conversation

JasonAllenCorns
Copy link
Contributor

refer to #12342; recovering from botched rebase...forked fresh and manually merged changes.

Tests passing.

@XhmikosR
Copy link
Member

@ResentedHook: you should just skip the revert commit alltogether or include it in the same patch.

@cvrebert
Copy link
Collaborator

We should also add this to the tooltip+popover docs.

@JasonAllenCorns
Copy link
Contributor Author

I can't include the CSS in the patch since the changes aren't mine (I don't understand how my grunt task would build this CSS package differently than the previous commiter).
Should I close this PR and open another where the CSS (and related revert) aren't included?

On May 13, 2014, at 5:07 PM, XhmikosR [email protected] wrote:

@ResentedHook: you should just skip the revert commit alltogether or include it in the same patch.


Reply to this email directly or view it on GitHub.

@cvrebert
Copy link
Collaborator

Don't bother. I'm fine with fixing up the commits before merging them. Just need @fat to give his final okay on this.

@XhmikosR
Copy link
Member

You should rebase then and skip that commit.

The reason this happens is because a change got in probably accidentally and no one has pushed a grunt dist patch yet.

primarily adds a data- attribute to the tooltip (and thus, the popover)
to create a self-reference.
@JasonAllenCorns
Copy link
Contributor Author

@XhmikosR , @cvrebert

ok, I think I've got this rebase figured out; skipped the variant CSS modification and just included the changes to tooltip.js and related tests.

@@ -166,6 +166,7 @@
.detach()
.css({ top: 0, left: 0, display: 'block' })
.addClass(placement)
.data('bs.' + this.type, this)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to the modal.js call to $this.data('bs.modal', ...), the tooltip has to accommodate the type - tooltip, popover, etc.

@XhmikosR
Copy link
Member

The rebase looks good now @ResentedHook :)

@JasonAllenCorns
Copy link
Contributor Author

@XhmikosR , @cvrebert - just curious; what's the next step, here?

@cvrebert
Copy link
Collaborator

  • We need to run the Sauce tests on this
  • @fat: Should this be publicly documented?

@fat
Copy link
Member

fat commented May 23, 2014

lg

@fat
Copy link
Member

fat commented May 23, 2014

Should this be publicly documented?

maybe lets be quiet about it for a while… and if it comes up a few times, we can add to docs?

@cvrebert
Copy link
Collaborator

Running Sauce test: https://travis-ci.org/twbs/bootstrap/jobs/25907822

@cvrebert
Copy link
Collaborator

Sauce tests passed. Merging per @fat's LG.

cvrebert added a commit that referenced this pull request May 23, 2014
Add tooltip self-reference to address #12320
@cvrebert cvrebert merged commit bc1ce42 into twbs:master May 23, 2014
@mdo mdo mentioned this pull request May 23, 2014
@JasonAllenCorns
Copy link
Contributor Author

hooray!

@rulatir
Copy link

rulatir commented Apr 16, 2015

Even in 3.2.0 it sometimes happens that $(event.target).data('bs.popover').$tip is undefined in a show.bs.popover handler (id does NOT happen in shown.bs.popover). I am investigating the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants