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.

Sign up for GitHub

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

Jump to bottom

Simplify the customization of popover #19415

Closed
fredgate opened this issue Mar 5, 2016 · 4 comments · Fixed by #31834 or #32217
Closed

Simplify the customization of popover #19415

fredgate opened this issue Mar 5, 2016 · 4 comments · Fixed by #31834 or #32217
Labels
feature has-pr help wanted js v4 v5
Projects
v5.0.0-beta1 v4.6.0

Comments

@fredgate
Copy link

fredgate commented Mar 5, 2016

This is a suggestion I have from a long time. I thought it was missing in version 3, and it might be well to allow it in version 4.
When we want to customize the popover (width, content padding...) we actually do not have other choice that to redefine the full template, which is not very readable, maintainable and evolutive.
A better option might be to allow to add a class to the popver, so we could add our custom styles to this class without impacting all the popovers.

Example :

.popover-custom .popover-content {
  padding: 0;
}

$('#foo').popover({
  title: ...,
  content: ...,
  class: 'popover-custom'
});
@RyanZim
Copy link

RyanZim commented Apr 4, 2016

I'm not sure what you're suggesting. Perhaps you could post a JSFiddle/JSBin of this in action?

@staticshock
Copy link

I second this.

Take a look at the myriad answers to this question on stack overflow to get a sense for how often it comes up and what kinds of workarounds people are resorting to: http://stackoverflow.com/questions/12170357/dynamically-add-a-class-to-bootstraps-popover-container

If I want to style a specific popover, I have several possible approaches:

  1. If I don't specify the popover's container, it's created adjacent to the target element. This gives me access to the popover from css via .target + .popover, but it also constrains the popover in some possibly breaking ways (e.g. overflow: hidden on an ancestor.) These things are annoying to work around in css.
  2. If I specify container: "body", it's appended to the <body>. This fixes the css problems inherent to the previous approach, but cripples my ability to style that specific popover from css.
  3. If I append a custom container to the body, I could use it via container: "#custom-container". This gives me access via #custom-container .popover, but has the downside of littering my base template with cruft like <div id="custom-container"></div>.
  4. If I append it to the body and specify a custom template, I may use .class-from-custom-template, but the custom template is a hunk of HTML I'd rather avoid writing if I can. The only interesting part of that template would be the class, and that's unclear when confronted with the entire template.
  5. I may resort to attaching the class from javascript, but that makes it hard to initialize popovers generically with a construct such as $('.has-popover').popover().

In the current state of things, I think the best solution is combining the 5th approach with some code that copies something like data-class from the target to the popover.

@mdo mdo added this to the v4.1 ideas milestone Nov 27, 2016
@awbergs
Copy link

awbergs commented Mar 3, 2017

What @fredgate proposes would be a pretty straightforward addition to the Tooltip.prototype.tip function in tooltip.js.

For example:

  Tooltip.prototype.tip = function () {
    if (!this.$tip) {
      this.$tip = $(this.options.template)

      // New className handling
      if(this.options.className) {
        this.$tip.addClass(this.options.className);
      }

      if (this.$tip.length != 1) {
        throw new Error(this.type + ' `template` option must consist of exactly 1 top-level element!')
      }
    }
    return this.$tip
  }

Not a huge change but I wonder if the root issue can be solved simply with more visible documentation of the .tip function in general.

I think any scenario can be covered using .tip() (please correct me if i'm wrong, I might not be considering all scenarios)

Single Use

  $('#my-popover').popover()
    .data('bs.popover')
    .tip()
    .addClass('my-class-name');

Multiple Matching Selectors

  $('.my-popovers').popover()
    .map(function(){
      $(this).data('bs.popover')
        .tip()
        .addClass('my-class-name')
    });

@mattcheah
Copy link

Was this ever addressed in V4? I've wanted a fix for this too and it seems like it could be done pretty quickly.

@mattcheah mattcheah mentioned this issue Sep 7, 2017
Closed
@mdo mdo removed this from the v4.1 ideas milestone Mar 4, 2019
@remeika remeika mentioned this issue Oct 5, 2020
Merged
@XhmikosR XhmikosR linked a pull request Nov 14, 2020 that will close this issue
tooltip/popover: add a customClass option #31834
Merged
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta1 via automation Nov 20, 2020
@XhmikosR XhmikosR added this to To do in v4.6.0 Nov 20, 2020
@XhmikosR XhmikosR moved this from To do to Inbox in v4.6.0 Nov 20, 2020
@XhmikosR XhmikosR moved this from Inbox to TODO in v5.0.0-beta1 Nov 20, 2020
@rohit2sharma95 rohit2sharma95 mentioned this issue Nov 20, 2020
Merged
@XhmikosR XhmikosR moved this from Inbox to Shipped in v4.6.0 Nov 21, 2020
@XhmikosR XhmikosR moved this from TODO to Inbox in v5.0.0-beta1 Nov 21, 2020
v5.0.0-beta1 automation moved this from Inbox to Done Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature has-pr help wanted js v4 v5
Projects
No open projects
v4.6.0
Shipped
v5.0.0-beta1
  
Done
Milestone
No milestone
8 participants
@staticshock @mdo @XhmikosR @awbergs @fredgate @mattcheah @RyanZim and others