Enhance popover arrow rendering when popover has a header #40994
Conversation
|
|
||
| // `: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 { |
There was a problem hiding this comment.
| &: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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We implament the popover with floating-ui
Oh yeah, got it! We plan to switch to it in v6 :)
Caution
Don't merge until ae129b2 is removed
Description
This PR tries to tackle #40993. The idea would be to change the color of the arrow only when there's a header, and when the popover is opened under the triggering element. That way, both background colors (arrow and headers) would be the same.
Given the order of the HTML elements, I haven't really found a way to do without using
:has, which is not "authorized" currently in our.browserslistrc. However, since it's an extra rule, it wouldn't affect the rendering of browsers not handling:has.What do you think @twbs/css-review?
Rendering
By default, with the modified doc commit:
By default, with the modified doc commit, and scrolling up:
Basic popovers without headers:
Type of changes
Checklist
npm run lint)Live previews
Related issues
Closes #40993