Fix Popover--right-bottom caret positioning #465
Conversation
| } | ||
|
|
||
| &::after { | ||
| bottom: 21px; | ||
| bottom: $spacer-3 + 1; |
There was a problem hiding this comment.
I'm trying to understand the changes here - is it true that margin-top usually takes care of the one-pixel-offset before before/after pseudo elements, but not in this case since these are positioned relative to the bottom, not top?
There was a problem hiding this comment.
Yup, it's true that margin-top doesn't affect absolutely positioned elements with bottom.
| @@ -108,7 +108,7 @@ | |||
| } | |||
|
|
|||
| &::before { | |||
| margin-top: -9px; | |||
| margin-top: -($spacer-2 + 1); | |||
There was a problem hiding this comment.
I know it's not a result of this PR, but it seems like this margin-top makes it so that the distance from the caret to the corner of the card is smaller for --right and --left popovers as compared to --top and --bottom popovers. Is this intended? Or should we have consistent distances from the carets to the corners of the popovers?
Smaller distance to corner:
Larger distance to corner:
We have some JS that tries to position the popovers relative to the hover target in a generic way, and it would be convenient if the distance from the caret to the corner was always consistent 😄. Thoughts?
There was a problem hiding this comment.
Honestly I'm not sure (I just got back from paternity leave, and my @primer/ds-core peeps are primarily responsible for this component), but my sense is that, yes, the horizontal and vertical alignment are intentionally different. I'd be very interested to see how that JS works, and how we can better support it from our side!
There was a problem hiding this comment.
For what it's worth, though, this value actually hasn't changed; I'm just calculating it as a nudge from a "known" value ($spacer-2: 8px) rather than hard-coding 9px.
There was a problem hiding this comment.
For what it's worth, though, this value actually hasn't changed; I'm just calculating it as a nudge from a "known" value ($spacer-2: 8px) rather than hard-coding 9px.
Right on - was asking an unrelated question 😄
Bummer that the distances aren't consistent, but we can handle it.
Thanks for taking this on!
There was a problem hiding this comment.
I know it's kind of crazy and might seem brittle, but you could determine the positions of those carets from their computed style, e.g.
const computed = window.getComputedStyle(myPopoverContent, '::after')
const bottom = computed.getPropertyValue('bottom')🙈
There was a problem hiding this comment.
I'm not sure of the reasons for the distances being different. @brandonrosage worked on adding this component but likely followed the custom styles that existed in dotcom. @pifafu wondering if you have any opinion on this with regards to using on hovercards?
There was a problem hiding this comment.
wondering if you have any opinion on this with regards to using on hovercards?
I didn't see a need for the distances between the corner of the card and the caret being different when the caret is top/bottom versus right/left for the user hovercard project. If the caret distance-to-corner was made consistent, we would be good with it 👍
The only spacing that the hovercards project wanted to update on was the distance of the hovercard from the entity that triggers it, since the original styles on the popover were flush to the triggering element.
There was a problem hiding this comment.
Thanks, @pifafu! Have you all done that in the implementation (e.g. with margin utility classes), or should we account for it in the component styles?
There was a problem hiding this comment.
@shawnbot our JS code positions the hovercard so that it's 12 pixels from the target element
There was a problem hiding this comment.
Have you all done that in the implementation (e.g. with margin utility classes), or should we account for it in the component styles?
@shawnbot I started with pb-2 to give more space when we started with the popover component, but @iancanderson's recent changes now help handle this! @califa also has interest in larger spacing between the triggering element on the popover component (mt-3 is too large, mt-2 is not enough?). It would be nice to have this accounted for in component styles 😄
shawnbot
changed the title
|
Okay, this is ready for a review now that #464 is merged. |
| @@ -108,7 +108,7 @@ | |||
| } | |||
|
|
|||
| &::before { | |||
| margin-top: -9px; | |||
| margin-top: -($spacer-2 + 1); | |||
There was a problem hiding this comment.
I'm not sure of the reasons for the distances being different. @brandonrosage worked on adding this component but likely followed the custom styles that existed in dotcom. @pifafu wondering if you have any opinion on this with regards to using on hovercards?
|
Ping @brandonrosage @pifafu 😊 |
|
Follow-on notes are in #476. Merging for 10.5! |
This fixes #462 and fleshes out all (?) of the popover positioning examples so that we can make sure they look right. (We're missing examples for
.Popover--right-bottom,.Popover--right-top,.Popover--left-bottom, and.Popover--left-top.) I've re-ordered the positioning examples (alphabetically) and linked to them from the modifier class listings earlier in the docs.Do not merge yet! This branches off #464 because we need markdown stories support to see the results without deploying to the style guide.