Skip to content

[PositionedOverlay] Add scroll support for all scroll containers #11399

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

Merged
sophschneider merged 4 commits into from
Feb 23, 2024
Merged

[PositionedOverlay] Add scroll support for all scroll containers #11399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35cbdb5
[PositionedOverlay] Add scroll support for all scroll containers
sophschneider Jan 5, 2024
749463d
Add horizontal scroll support
sophschneider Jan 5, 2024
5aa75b1
Remove horizontal scroll fix
sophschneider Feb 21, 2024
7df0cd1
Revert "Remove horizontal scroll fix"
sophschneider Feb 21, 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
5 changes: 5 additions & 0 deletions .changeset/olive-forks-call.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Added `PositionedOverlay` scroll support for all scroll containers
48 changes: 48 additions & 0 deletions polaris-react/src/components/Popover/Popover.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
EmptySearchResult,
Text,
BlockStack,
Card,
InlineStack,
} from '@shopify/polaris';
import {SearchIcon} from '@shopify/polaris-icons';

Expand Down Expand Up @@ -832,6 +834,52 @@ export function WithSubduedPane() {
);
}

export function WithScrollContainer() {
const [popoverActive, setPopoverActive] = useState(true);

const togglePopoverActive = useCallback(
() => setPopoverActive((popoverActive) => !popoverActive),
[],
);

const activator = (
<Button onClick={togglePopoverActive} disclosure size="large">
Scroll me!
</Button>
);

return (
<Card>
<div style={{height: '300px', width: '300px', overflow: 'scroll'}}>
<div style={{height: '400px', width: '400px', overflow: 'hidden'}}>
<Box paddingBlock="1000" />
<InlineStack>
<Box paddingInline="1000" />
<Popover
active={popoverActive}
activator={activator}
onClose={togglePopoverActive}
ariaHaspopup={false}
sectioned
>
<Popover.Pane>
<Box padding="400">
<Text as="p">Popover content</Text>
</Box>
</Popover.Pane>
<Popover.Pane subdued>
<Box padding="400">
<Text as="p">Subdued popover pane</Text>
</Box>
</Popover.Pane>
</Popover>
</InlineStack>
</div>
</div>
</Card>
);
}

const StopPropagation = ({children}: React.PropsWithChildren<any>) => {
const stopEventPropagation = (event: React.MouseEvent | React.TouchEvent) => {
event.stopPropagation();
Expand Down
45 changes: 34 additions & 11 deletions polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {classNames} from '../../utilities/css';
import {getRectForNode, Rect} from '../../utilities/geometry';
// eslint-disable-next-line import/no-deprecated
import {EventListener} from '../EventListener';
import {Scrollable} from '../Scrollable';
import {layer, dataPolarisTopBar} from '../shared';

import {
Expand Down Expand Up @@ -198,20 +197,44 @@ export class PositionedOverlay extends PureComponent<
this.overlay = node;
};

Copy link
Contributor Author

@sophschneider sophschneider Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently this is built to add vertically scrolling containers and not horizontally scrolling containers, but horizontal scroll support can be easily added if needed.

Performance is actually better than before since Scrollable.forNode was repeating a DOM search for iteration of the while loop. We are now doing single traversal while loop.

Copy link
Contributor

@marty-Wallace marty-Wallace Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah interesting. If there are enough rows in the bulk editor to cause the page to be vertically scrollable, then the popovers are also tracking with horizontal scrolling (I'm guessing since it is the same container that scrolls). But without a vertical scrollbar, the popovers do not track horizontally.

e.g.

Thoughts on adding horizontal containers as well?

Copy link
Contributor Author

@sophschneider sophschneider Jan 5, 2024

Choose a reason for hiding this comment

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

ill add them and see what it does! Wanted a good use case for it and the bulk editor is perfect :)

Copy link
Contributor Author

@sophschneider sophschneider Jan 8, 2024 •
edited by chloerice
Loading

Choose a reason for hiding this comment

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

@marty-Wallace good news! I got it to work for horizontal scroll for both your examples above, but theres a problem with the bulk editor where the scroll container is registered as 100% full screen width behind the sticky column so it doesn't close the popover until the end of that scroll container. I can't really think of a way to fix this from polaris' end so we might have to modify the width of the bulk editor scroll container or make the sticky column z-index higher than the popover

horizontal-scroll.mov

Copy link
Contributor

@marty-Wallace marty-Wallace Jan 8, 2024

Choose a reason for hiding this comment

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

@sophschneider nice!

Any thoughts on this @lbenie or @mattkubej?

IMO - this is an improvement over the existing behaviour and we can fix the popover showing over the sticky column in the bulk editor as a follow up?

Copy link

@e7chun e7chun Jan 10, 2024

Choose a reason for hiding this comment

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

Gotcha -- So 2 separate PRs but they both go out in the same polaris version? That's fine with me 👍

Copy link
Member

@chloerice chloerice Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since the popover staying visible when the activator is behind the sticky columns is already a bug in production and not introduced by these changes, we shouldn't block this PR from shipping now. @sophschneider

Copy link
Contributor Author

@sophschneider sophschneider Feb 13, 2024

Choose a reason for hiding this comment

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

@chloerice do we need it for your HoverCard PR? I was holding off on it in case there were conflicts and wanted you to merge in first

Copy link
Member

@chloerice chloerice Feb 15, 2024

Choose a reason for hiding this comment

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

I do need it, though it's not blocking. No worries about conflicts, should be minimal since we didn't make changes to the same part of the component 🙏🏽

Copy link
Contributor Author

@sophschneider sophschneider Feb 22, 2024

Choose a reason for hiding this comment

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

Hey all! As Chloe mentioned, this is less of a regression in the bulk editor and more of a side step to the existing experience.

When I mentioned

might have to modify the width of the bulk editor scroll container or make the sticky column z-index higher than the popover

above, I explored both solutions and they aren't simple or fast to do as the bulk editor table is built by rows, not columns. So to elevate the sticky column above the popover, we would have semantically change the bulk editor table which is quite a big task.

I have some other solutions for the bulk editor team such as closing the popovers on scroll, but this would have to be implemented on a popover by popover basis in web.

If we don't get any major pushback, I'm going to go ahead an merge this to unblock other overlay work our team is doing

private setScrollableContainers = () => {
const containers: (HTMLElement | Document)[] = [];
let scrollableContainer = Scrollable.forNode(this.props.activator);
private isScrollContainer = (node: HTMLElement) => {
const yOverflow = node.scrollHeight > node.clientHeight;
const xOverflow = node.scrollWidth > node.clientWidth;

if (scrollableContainer) {
containers.push(scrollableContainer);
if (!yOverflow && !xOverflow) {
return false;
}

while (scrollableContainer?.parentElement) {
scrollableContainer = Scrollable.forNode(
scrollableContainer.parentElement,
);
const styles = window.getComputedStyle(node);
const yScroll =
styles.overflowY === 'auto' || styles.overflowY === 'scroll';
const xScroll =
styles.overflowX === 'auto' || styles.overflowX === 'scroll';

containers.push(scrollableContainer);
if ((yOverflow && yScroll) || (xOverflow && xScroll)) {
return true;
}

return false;
};

private setScrollableContainers = () => {
const containers: (HTMLElement | Document)[] = [];
let currentNode: HTMLElement | ParentNode | null = this.props.activator;

while (currentNode !== null) {
if (
currentNode instanceof HTMLElement &&
this.isScrollContainer(currentNode)
) {
containers.push(currentNode);
}

currentNode = currentNode.parentNode;
}

if (!containers.length) {
containers.push(document);
}

this.scrollableContainers = containers;
Expand Down
7 changes: 6 additions & 1 deletion polaris-react/src/components/PositionedOverlay/utilities/math.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,12 @@ export function calculateHorizontalPosition(
export function rectIsOutsideOfRect(inner: Rect, outer: Rect) {
const {center} = inner;

return center.y < outer.top || center.y > outer.top + outer.height;
return (
center.y < outer.top ||
center.y > outer.top + outer.height ||
inner.left < outer.left ||
inner.left + inner.width > outer.left + outer.width
Copy link
Contributor Author

@sophschneider sophschneider Jan 8, 2024

Choose a reason for hiding this comment

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

This closes the popover once it reaches the left or right side of the activator. This works with all alignments of the popover and I think looks nicer than closing when reaching the center of the activator

);
}

export function intersectionWithViewport(
Expand Down