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

sophschneider · 2024-01-04T21:26:37Z

WHY are these changes introduced?

Part of https://github.com/Shopify/polaris-internal/issues/1246

WHAT is this pull request doing?

Before, the PositionedOverlay component (used by Popover and Tooltip) only scrolled with Polaris Scrollable components or the window.

This PR adds support for all scroll containers so that Popover and Tooltip will always scroll with containers, even if its not wrapped in a Scrollable.

Before	After

Alternative approaches

I picked this approach since it decouples PositionedOverlay from Scrollable and doesn't require any extra setup or knowledge from consumers, it should "just work". If this approach is undesirable, we could also either:

Extend Tooltip and Popover to accept a ref to custom scrollable containers and keep automatic Scrollable detection
Export data-polaris-scrollable or another more generic attribute that consumers can add to their custom scroll elements. Then update Tooltip and Popover documentation to require proper scroll container setup
Extend the Scrollable component to support the scrolling features in the settings modal, refactor the settings modal to use Scrollable, and update Tooltip and Popover documentation to require that all scroll containers are Scrollables

How to 🎩

Go to Taxes settings
Click "Sort" on the Manage sales tax collection card
Scroll so the "Sort" button moves
Make sure popover moves with button and closes when the button leaves the viewport

🎩 checklist

Tested a snapshot
Tested on mobile
Tested on multiple browsers

sophschneider · 2024-01-05T20:02:15Z

polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx

  private setOverlay = (node: HTMLElement | null) => {
    this.overlay = node;
  };



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.

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.

No vertical scrollbar:

With vertical scrollbar

Thoughts on adding horizontal containers as well?

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

@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

@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?

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

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

@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

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 🙏🏽

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

sophschneider · 2024-01-05T20:07:56Z

/snapit

github-actions · 2024-01-05T20:11:25Z

🫰✨ Thanks @sophschneider! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20240105201004

yarn add @shopify/polaris@0.0.0-snapshot-release-20240105201004

yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20240105201004

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20240105201004

marty-Wallace · 2024-01-05T20:43:40Z

@sophschneider could you add a few more products to your spin instance. I'd like to test out how this scrolling change affects some of the columns in the BulkEditor as well.

rake dev:products:create NUM=30 should do it.

sophschneider · 2024-01-05T20:52:47Z

@marty-Wallace done!

marty-Wallace · 2024-01-05T21:15:51Z

cc: @mattkubej - this PR should fix popover scrolling in the bulk editor so the popovers track with their cells.

sophschneider · 2024-01-05T22:10:30Z

/snapit

github-actions · 2024-01-05T22:13:34Z

🫰✨ Thanks @sophschneider! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20240105221218

yarn add @shopify/polaris@0.0.0-snapshot-release-20240105221218

yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20240105221218

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20240105221218

sophschneider · 2024-01-08T20:39:44Z

/snapit

github-actions · 2024-01-08T20:42:52Z

🫰✨ Thanks @sophschneider! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20240108204136

yarn add @shopify/polaris@0.0.0-snapshot-release-20240108204136

yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20240108204136

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20240108204136

sophschneider · 2024-01-08T21:12:48Z

polaris-react/src/components/PositionedOverlay/utilities/math.ts

+    center.y < outer.top ||
+    center.y > outer.top + outer.height ||
+    inner.left < outer.left ||
+    inner.left + inner.width > outer.left + outer.width


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

marty-Wallace

Nice work on this!

🎩'd and looks good to me, I'll leave it up to the core operate folks to determine how to make the bulk editor change in coordination with this?

chloerice

Awesome, thanks for fixing this @sophschneider!! I can ping you or the bulk editor folks who have context on the z-index change needed to fix the sticky column issue in the upgrade PR (I've got one prepped for 12.11.0, but will be waiting to ship it until we get the backward compatibilty PR shipped for BulkActions/SelectAllActions)

sophschneider · 2024-02-21T15:49:29Z

/snapit

github-actions · 2024-02-21T15:52:43Z

✨ Thanks @sophschneider! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-icons@0.0.0-snapshot-20240221155159

yarn add @shopify/polaris@0.0.0-snapshot-20240221155159

sophschneider · 2024-02-21T19:14:56Z

/snapit

github-actions · 2024-02-21T19:18:01Z

✨ Thanks @sophschneider! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-icons@0.0.0-snapshot-20240221191730

yarn add @shopify/polaris@0.0.0-snapshot-20240221191730

This reverts commit 5aa75b1.

@sophschneider

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@12.19.0 ### Minor Changes - [#11399](#11399) [`0134d2378`](0134d23) Thanks [@sophschneider](https://github.com/sophschneider)! - Added `PositionedOverlay` scroll support for all scroll containers - [#11622](#11622) [`1f81501c8`](1f81501) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated the BulkActions component to include logic to handling selecting and deselecting rows - [#11637](#11637) [`1ac638246`](1ac6382) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated Pagination table variant to have more prominent and centrally-aligned actions ### Patch Changes - [#11644](#11644) [`b95fc9807`](b95fc98) Thanks [@kyledurand](https://github.com/kyledurand)! - Removed nav wrapper from breadcrumbs since it now only renders a single link ## polaris.shopify.com@0.63.2 ### Patch Changes - Updated dependencies \[[`0134d2378`](0134d23), [`1f81501c8`](1f81501), [`1ac638246`](1ac6382), [`b95fc9807`](b95fc98)]: - @shopify/polaris@12.19.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ers" (#11659) Reverts #11399 Reverting because of some issues with the store switcher. Will remerge when we fix the bugs

…pify#11399) ### WHY are these changes introduced? Part of Shopify/polaris-internal#1246 ### WHAT is this pull request doing? Before, the `PositionedOverlay` component (used by `Popover` and `Tooltip`) only scrolled with Polaris `Scrollable` components or the window. This PR adds support for all scroll containers so that `Popover` and `Tooltip` will always scroll with containers, even if its not wrapped in a `Scrollable`. |Before|After| |-|-| |![Before-Scroll-Popover](https://github.com/Shopify/polaris/assets/20652326/f96652a0-bd20-4305-899e-6242ebb30496)|![After-Scroll-Popover](https://github.com/Shopify/polaris/assets/20652326/55cb30df-90e5-45b6-b337-819e9e05e376)| |![before-taxes-overlay](https://github.com/Shopify/polaris/assets/20652326/c421bee0-0d64-4a06-a291-ee7e59d65382)|![after-taxes-overlay](https://github.com/Shopify/polaris/assets/20652326/d66cbfc5-7298-47f3-bd7a-a3b8e2d0a364)| ### Alternative approaches I picked this approach since it decouples `PositionedOverlay` from `Scrollable` and doesn't require any extra setup or knowledge from consumers, it should "just work". If this approach is undesirable, we could also either: 1. Extend `Tooltip` and `Popover` to accept a `ref` to custom scrollable containers and keep automatic `Scrollable` detection 2. Export `data-polaris-scrollable` or another more generic attribute that consumers can add to their custom scroll elements. Then update `Tooltip` and `Popover` documentation to require proper scroll container setup 3. Extend the `Scrollable` component to support the scrolling features in the settings modal, refactor the settings modal to use `Scrollable`, and update `Tooltip` and `Popover` documentation to require that all scroll containers are `Scrollable`s ### How to 🎩 * https://5d559397bae39100201eedc1-fdxylxukuv.chromatic.com/?path=/story/all-components-popover--with-scroll-container * https://admin.web.web-ya2s.sophie-schneider.us.spin.dev/store/shop1/settings/taxes 1. Go to Taxes settings 4. Click "Sort" on the `Manage sales tax collection` card 5. Scroll so the "Sort" button moves 6. Make sure popover moves with button and closes when the button leaves the viewport ### 🎩 checklist - [x] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)

sophschneider force-pushed the sophie/popover-scroll-fix branch 3 times, most recently from 17d7ccb to dbecf98 Compare January 5, 2024 19:28

Shopify deleted a comment from github-actions bot Jan 5, 2024

sophschneider force-pushed the sophie/popover-scroll-fix branch from dbecf98 to 719d218 Compare January 5, 2024 20:00

sophschneider commented Jan 5, 2024

View reviewed changes

sophschneider force-pushed the sophie/popover-scroll-fix branch from 719d218 to d49229a Compare January 5, 2024 20:07

Shopify deleted a comment from github-actions bot Jan 5, 2024

sophschneider marked this pull request as ready for review January 5, 2024 20:29

sophschneider requested review from kyledurand and marty-Wallace January 5, 2024 20:36

sophschneider force-pushed the sophie/popover-scroll-fix branch from f8c1d12 to 81c90c4 Compare January 8, 2024 20:39

sophschneider force-pushed the sophie/popover-scroll-fix branch from 81c90c4 to ff08934 Compare January 8, 2024 21:06

sophschneider commented Jan 8, 2024

View reviewed changes

sophschneider force-pushed the sophie/popover-scroll-fix branch from ff08934 to 31a2c16 Compare January 8, 2024 21:28

marty-Wallace approved these changes Jan 11, 2024

View reviewed changes

sophschneider requested review from chloerice, e7chun, kristy-chen2023 and mattkubej January 11, 2024 18:23

chloerice approved these changes Jan 19, 2024

View reviewed changes

lgriffee added the #gsd:38846 Admin Quality Improvements (Q1 2024) label Jan 25, 2024

sophschneider added 2 commits February 21, 2024 15:49

[PositionedOverlay] Add scroll support for all scroll containers

35cbdb5

Add horizontal scroll support

749463d

sophschneider force-pushed the sophie/popover-scroll-fix branch from 31a2c16 to 749463d Compare February 21, 2024 15:49

Remove horizontal scroll fix

5aa75b1

Revert "Remove horizontal scroll fix"

7df0cd1

This reverts commit 5aa75b1.

sophschneider merged commit 0134d23 into main Feb 23, 2024

sophschneider deleted the sophie/popover-scroll-fix branch February 23, 2024 14:34

shopify-github-actions-access bot mentioned this pull request Feb 23, 2024

Version Packages #11649

Merged

sophschneider mentioned this pull request Feb 27, 2024

Revert "[PositionedOverlay] Add scroll support for all scroll containers" #11659

Merged

sophschneider added a commit that referenced this pull request Feb 27, 2024

Revert "[PositionedOverlay] Add scroll support for all scroll contain…

0371de8

…ers" (#11659) Reverts #11399 Reverting because of some issues with the store switcher. Will remerge when we fix the bugs

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

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

Uh oh!

Conversation

sophschneider commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

Alternative approaches

How to 🎩

🎩 checklist

Uh oh!

sophschneider Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marty-Wallace Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophschneider Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

sophschneider Jan 8, 2024 • edited by chloerice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marty-Wallace Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

e7chun Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

chloerice Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophschneider Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

chloerice Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

sophschneider Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

sophschneider commented Jan 5, 2024

Uh oh!

github-actions bot commented Jan 5, 2024

Uh oh!

marty-Wallace commented Jan 5, 2024

Uh oh!

sophschneider commented Jan 5, 2024

Uh oh!

marty-Wallace commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sophschneider commented Jan 5, 2024

Uh oh!

github-actions bot commented Jan 5, 2024

Uh oh!

sophschneider commented Jan 8, 2024

Uh oh!

github-actions bot commented Jan 8, 2024

Uh oh!

sophschneider Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

marty-Wallace left a comment

Choose a reason for hiding this comment

Uh oh!

chloerice left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophschneider commented Feb 21, 2024

Uh oh!

github-actions bot commented Feb 21, 2024

Uh oh!

sophschneider commented Feb 21, 2024

sophschneider commented Jan 4, 2024 •

edited

Loading

sophschneider Jan 5, 2024 •

edited

Loading

marty-Wallace Jan 5, 2024 •

edited

Loading

sophschneider Jan 8, 2024 •

edited by chloerice

Loading

chloerice Feb 13, 2024 •

edited

Loading

marty-Wallace commented Jan 5, 2024 •

edited

Loading

chloerice left a comment •

edited

Loading