Add block-start and block-end positioning to PopoverMixin #5125
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
If possible, I would like to build out the visual-diffs in a separate PR. |
| @@ -53,14 +73,56 @@ export const PopoverMixin = superclass => class extends superclass { | |||
| :host([_opened]) { | |||
| display: inline-block; | |||
| } | |||
| :host([_location="block-start"]) { | |||
There was a problem hiding this comment.
These logical positioning terms are being used to align more closely with what the browser is doing, as well as to set us up for supporting other locations.
block-start -> top (this PR)
block-end -> bottom (this PR)
inline-start -> left (future PR)
inline-end -> right (future PR)
| } | ||
|
|
||
| .pointer { | ||
| clip: rect(-5px, 21px, 8px, -7px); |
There was a problem hiding this comment.
We were able to ditch some old pointer CSS with our fixed positioning. 🎉
| @@ -72,26 +134,37 @@ export const PopoverMixin = superclass => class extends superclass { | |||
| animation: var(--d2l-popover-animation-name, var(--d2l-popover-default-animation-name)) 300ms ease; | |||
| } | |||
| } | |||
|
|
|||
| :host([_offscreen]) { | |||
There was a problem hiding this comment.
As with fixed position dropdown, we position offscreen when the opener is scrolled out of view.
| } | ||
|
|
||
| _clearDismissible() { | ||
| #addRepositionHandlers() { |
There was a problem hiding this comment.
This code is basically copied from DropdownContentMixin. The only real difference is that we need to handle the opener IntersectionObserver here, since the opener could literally be any element, as compared to dropdown, where we do this wire-up in DropdownOpenerMixin.
| if (!this._dismissibleId) return; | ||
| clearDismissible(this._dismissibleId); | ||
| this._dismissibleId = null; | ||
| } | ||
|
|
||
| _focusContent(container) { | ||
| #constrainSpaceAround(spaceAround, spaceRequired, openerRect) { |
There was a problem hiding this comment.
This is basically adapted from DropdownContentMixin, except the boundary constraints aren't included here (yet), and align is changed to position span to align with what the browser is doing for positioning in the logical 3x3 grid.
| return this.shadowRoot.querySelector('.content-container'); | ||
| } | ||
|
|
||
| #getLocation(spaceAround, spaceAroundScroll, spaceRequired) { |
There was a problem hiding this comment.
This method basically defines where we will place the popover based on the specified preferred location, based on the space available in the viewport and document.
| return this.shadowRoot.querySelector('.pointer'); | ||
| } | ||
|
|
||
| #getPointerPosition(openerRect) { |
There was a problem hiding this comment.
This logic is almost exactly the same as DropdownContentMixin but I adjusted the calculation for the RTL align/span cases since I noticed they were not working quite right.
| return position; | ||
| } | ||
|
|
||
| #getPosition(spaceAround, openerRect, contentRect) { |
There was a problem hiding this comment.
This logic is the same as DropdownContentMixin, with some renaming.
| return position; | ||
| } | ||
|
|
||
| #getPositionXAdjustment(spaceAround, openerRect, contentRect) { |
There was a problem hiding this comment.
This logic is the same as DropdownContentMixin with some renaming.
| } | ||
|
|
||
| _renderPopover() { | ||
| const content = html`<div class="content"><slot></slot></div>`; | ||
| async #position(contentRect, options) { |
There was a problem hiding this comment.
This logic is the same as DropdownContentMixin with some renaming. Was also nice to ditch the non-fixed positioning logic.
| const isSupported = ('popover' in HTMLElement.prototype); | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.log('Popover', isSupported); | ||
|
|
||
| export const PopoverMixin = superclass => class extends superclass { | ||
| export const PopoverMixin = superclass => class extends RtlMixin(superclass) { |
There was a problem hiding this comment.
I was hoping we'd be able to EOL/remove RtlMixin at some point in favour of logical properties everywhere, but I see this is using it for something else. Wondering if we could maybe introduce something else that doesn't result in a reactive dir prop sprouting on the host?
There was a problem hiding this comment.
Hmmm, it looks like inset-inline-start is the replacement for left. Likewise, inset-inline-end corresponds to right when LTR. Browser support is (CanIUse):
Chrome >=87 (legacy)
Safari >= 14.1 (legacy)
Firefox >= 63 (blocked)
That's what we'd ideally use, but the positioning behaviour would be completely broken in earlier browsers. It would probably be displayed in top corner, but we maybe it could be set up with fallback to center (the browser's popover default). Maybe we could position using margin logical properties since they are supported by earlier browser versions. But that doesn't feel much better to me.
I'm not confident the position calculations would work as-is. They might need to be revamped, which I think would be "ok". Having just spent a lot of time on the code, I'm fairly current on what they are all doing.
There was a problem hiding this comment.
I'll send you the new tiers that start in January privately.
| this._noAutoFocus = properties?.noAutoFocus ?? false; | ||
| this._noPointer = properties?.noPointer ?? false; | ||
| this._offset = properties?.offset ?? 16; | ||
| this._preferredPosition = { |
There was a problem hiding this comment.
Not sure how often we're expecting configure to be called, but this line is going to result in a re-render 100% of the time, even when location/span/allowFlip don't change.
There was a problem hiding this comment.
That's a good point. The way I have the test consumer set up, configure should only get called when one of these properties has changed and a re-render will be necessary. While that's how I'd expect consumers to be set up, it's entirely possible for them to unnecessarily call configure, and we can probably guard against that here.
| const xAdjustment = Math.min(20 + ((pointerRotatedLength - pointerLength) / 2), (openerRect.width - pointerLength) / 2); | ||
| if (!isRTL) { | ||
| if (this._preferredPosition.span === 'end') { | ||
| position.left = openerRect.left + xAdjustment; |
There was a problem hiding this comment.
If we used inset-inline-start/inset-inline-end instead, could we stop needing to know about isRTL?
There was a problem hiding this comment.
Haha, yeah. Added reply to your other comment. Really just comes down to browser support.
GAUD-6456 (positioning)
GAUD-7121 (sizing)
GAUD-7120 (pointer)
This PR adds
block-start(above) andblock-end(end) positioning to thePopoverMixin. The fixed positioning logic was adapted fromDropdownContentMixin. It also adds sizing logic since that's necessary for the positioning calculations.It does not include
inline-start(left) andinline-end(right) positioning. That will be added in a future PR.The
PopoverMixindemo is not in ourindex.html. Here is a link to it.