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

Enhance popover arrow rendering when popover has a header #40994

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Enhance popover arrow rendering when popover has a header #40994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e34813f
Enhance popover arrow rendering when popover has a header
julien-deramond Oct 29, 2024
ae129b2
Remove this commit before merging
julien-deramond Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions scss/_popover.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@
top: var(--#{$prefix}popover-border-width);
border-bottom-color: var(--#{$prefix}popover-bg);
}

// `:has` is not yet supported by our `.browserlistrc` but since it adds an extra layer of specificity,
// it doesn't hurt the browsers that don't support it.
&:has(+ .popover-header)::after {
Copy link

Choose a reason for hiding this comment

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

Suggested change
&:has(+ .popover-header)::after {
&:has(+ .popover-header)::after,
&:has(+ * .popover-header)::after {

I'd would be nice to have as a library builder. Since our library has a div there inbetween to controll the size.

see: https://github.com/bootstrap-vue-next/bootstrap-vue-next/blob/main/packages/bootstrap-vue-next/src/components/BPopover/BPopover.vue#L24

Copy link
Member Author

Choose a reason for hiding this comment

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

Let us think about it with @twbs/css-review, but my initial impression is that your usage seems specific to your downstream library, meaning it should handle this styling specificity independently by either:

  • Adding its own overrides with &:has(+*.popover-header)::after
  • Adjusting the DOM structure to align with our component's expected layout (probably not doable in that case on your side)

Let me walk you through my thought process. Incorporating this rule directly into Bootstrap would indeed benefit your library. However, I'm concerned that adding * selectors (or similar rules) to support various downstream libraries could lead to additional constraints. Furthermore, this rule isn't inherently tested or visible within our environment, so there's a risk it could eventually be removed as redundant, given its lack of relevance to our internal use cases.

Additionally, if you later adjust the structure and no longer need this rule, it would remain in our codebase indefinitely, serving no purpose for anyone.

Copy link

Choose a reason for hiding this comment

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

Undertandable. I think I need to check how we could build a better base for our scss if someone wants to customize. The css variables make this a lot easier!

We implament the popover with floating-ui and it offers more features to customize the sizing and so on, and i made the popover scrollable for larger content. I had some weirdness implementing the sizing in the main popover div, so that's why I added the internal div.

Copy link
Member Author

Choose a reason for hiding this comment

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

We implament the popover with floating-ui

Oh yeah, got it! We plan to switch to it in v6 :)

--#{$prefix}popover-bg: var(--#{$prefix}popover-header-bg);
}
}

// This will remove the popover-header's border just below the arrow
Expand Down
2 changes: 1 addition & 1 deletion site/content/docs/5.3/components/popovers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ We use JavaScript similar to the snippet above to render the following live popo
{{< /callout >}}

{{< example stackblitz_add_js="true" >}}
<button type="button" class="btn btn-lg btn-danger" data-bs-toggle="popover" data-bs-title="Popover title" data-bs-content="And here's some amazing content. It's very engaging. Right?">Click to toggle popover</button>
<button type="button" class="btn btn-lg btn-danger" data-bs-toggle="popover" data-bs-title="Popover title" data-bs-content="And here's some amazing content. It's very engaging. Right?" data-bs-placement="bottom">Click to toggle popover</button>
{{< /example >}}

### Four directions
Expand Down