[Scrollable] refactor: rewrite Scrollable utilizing platform features #7443

iAmNathanJ · 2022-10-18T21:18:47Z

WHY are these changes introduced?

This updates the Scrollable component to be a function component, but also changes the fundamental implementation strategy.

It no longer controls the DOM node's scroll, allowing the platform to handle all scrolling interaction and only using native features to supply enhancements for the ScrollTo component and the scroll hint functionality.

Primary Benefits:

It allows for smooth scrolling in React 18 on iOS. Since we are no longer setting scroll position on the target element as a side effect of state, it has no conflict with automatic batching by React. See Scrollable broken with React 18 automatic batching #7424
There are minimal wasted renders when scrolling. Previously, we would render many times just by virtue of scrolling.

resolves #7424

WHAT is this pull request doing?

Before/After 👀

Before

Momentum scrolling interrupted by render/commit cycles

Video.1.mov

After

Smooth scrolling

Video_1.mov

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Access storybook on an iOS device:

Using iOS device, navigate to the Scrollable component:
http://<your.network.ip>:6006/?path=/story/all-components-scrollable--default&globals=profiler:true

On main branch: scrolling should be janky - experience is choppy, momentum gets interrupted
On this branch: scrolling should be smooth
- shadow prop works as intended
- hint prop works as intended
- ScrollTo component works within Scrollable

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

github-actions · 2022-10-18T23:43:37Z

size-limit report 📦

Path	Size
polaris-react-cjs	209.22 KB (-0.13% 🔽)
polaris-react-esm	135.68 KB (-0.15% 🔽)
polaris-react-esnext	191.23 KB (-0.14% 🔽)
polaris-react-css	41.71 KB (-0.03% 🔽)

iAmNathanJ · 2022-10-19T19:41:32Z

/snapit

github-actions · 2022-10-19T19:46:02Z

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221019194357

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

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

BPScott · 2022-10-20T17:34:14Z

Bomp bomp bomp, another class component bites the dust #1995

kyledurand

This looks fantastic but I haven't had a chance to finish tophatting in the admin yet. I'm also trying to get a couple more folks with context to take a look in the meantime

.changeset/many-otters-repeat.md

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

AndrewMusgrave

Looking good 🙌 Left a few comments to gain some context!

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

kyledurand · 2022-10-21T16:06:50Z

I'm probably going to ship a bug fix to scrollable today: #7481

Can you include that fix in this PR before you ship?

iAmNathanJ · 2022-10-21T17:55:33Z

I'm probably going to ship a bug fix to scrollable today: #7481

Can you include that fix in this PR before you ship?

@kyledurand yeah, looks good. I'll rebase this and resolve any conflicts.

This updates the Scrollable component to be a function component, but also changes the fundamental implementation strategy. Previously, the Scrollable component held internal state for the scroll position and controlled the scroll of the actual DOM node as a side effect. With this change, there is no controlling of the DOM node's scroll, allowing the platform to handle all scrolling interaction and only using native features to supply enhancements for the ScrollTo component, the hint functionality.

- remove unneeded css class - respect reduced motion settings in scrollTo function - use lazy ref for sticky manager - fix: scroll hint happens at max once per mount - fix: invoke callback when scrolled to bottom of container

polaris-react/src/components/Scrollable/Scrollable.stories.tsx

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

tmlayton

Couple minor comments, otherwise 👍

…handlers

AndrewMusgrave

Nice work, LGTM 🙌 Left a comment re: a dependency on use effect that we'll want to keep an eye on!

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

iAmNathanJ · 2022-10-27T16:40:57Z

/snapit

github-actions · 2022-10-27T16:46:16Z

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221027164342

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

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

kyledurand

👏 Code looks good. Haven't 🎩 'd the latest snapshot though

kyledurand · 2022-10-31T17:28:30Z

.changeset/many-otters-repeat.md

+'@shopify/polaris': patch
+---
+
+Improve performance of the Scrollable component with React 18


We try to keep entries in past tense

Suggested change

Improve performance of the Scrollable component with React 18

Improved performance of the Scrollable component with React 18

wowzers, I missed this comment before I merged. Sorry about that!

@anthonymenecola

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-icons@6.5.0 ### Minor Changes - [#7548](#7548) [`432bdd5fe`](432bdd5) Thanks [@anthonymenecola](https://github.com/anthonymenecola)! - add cancel major icon - [#7620](#7620) [`35be8a003`](35be8a0) Thanks [@rdott](https://github.com/rdott)! - Added inactive location minor and major icons ## @shopify/polaris-migrator@0.8.0 ### Minor Changes - [#7403](#7403) [`8859f5db5`](8859f5d) Thanks [@jesstelford](https://github.com/jesstelford)! - Introduce `migrate-motion` migration for migrating `transition`, `transition-duration`, and `transition-delay` usages of duration values. - [#7606](#7606) [`cf7badbd1`](cf7badb) Thanks [@samrose3](https://github.com/samrose3)! - Renamed and split migrations based on scope and type (react, scss, and styles) - [#7543](#7543) [`8c1989618`](8c19896) Thanks [@jesstelford](https://github.com/jesstelford)! - Expose utilities for SASS Migrations to leverage the Suggestion-on-partial-fix pattern - [#7529](#7529) [`3652eb901`](3652eb9) Thanks [@samrose3](https://github.com/samrose3)! - Add relative option for replace-text-component migration - [#7532](#7532) [`ba576498d`](ba57649) Thanks [@jesstelford](https://github.com/jesstelford)! - Expose the .report() method to SASS migrations for easier aggregation of discovered issues during a migration run. ### Patch Changes - [#7606](#7606) [`cf7badbd1`](cf7badb) Thanks [@samrose3](https://github.com/samrose3)! - Update `createInlineComment` to format text with RegExp - [#7606](#7606) [`cf7badbd1`](cf7badb) Thanks [@samrose3](https://github.com/samrose3)! - Add support to replace Identifiers along with JSXIdentifiers for Text migration - [#7632](#7632) [`1f2ec8bfe`](1f2ec8b) Thanks [@samrose3](https://github.com/samrose3)! - Check for targeted component import before modifying in Text component migration - Updated dependencies \[[`6e9edd3b5`](6e9edd3)]: - @shopify/polaris-tokens@6.3.0 ## @shopify/polaris@10.11.0 ### Minor Changes - [#7572](#7572) [`20c8cad81`](20c8cad) Thanks [@laurkim](https://github.com/laurkim)! - Replaced usage of text components in component stories with `Text` component - [#7621](#7621) [`6e9edd3b5`](6e9edd3) Thanks [@aveline](https://github.com/aveline)! - - Added border width prop to `Box` - Exported color token subset alias types from tokens package and remove from `Box` - [#7068](#7068) [`ccdcea22e`](ccdcea2) Thanks [@laurkim](https://github.com/laurkim)! - Deprecated `DisplayText`, `Heading`, `Subheading`, `Caption`, `TextStyle`, and `VisuallyHidden` components ### Patch Changes - [#7644](#7644) [`b3e73ee04`](b3e73ee) Thanks [@kyledurand](https://github.com/kyledurand)! - Added horizontal spacing defaults to `Bleed` - [#7530](#7530) [`79d92a820`](79d92a8) Thanks [@samrose3](https://github.com/samrose3)! - Replaced all typography components with the new `Text` component. Added support for `text-inverse` color type on `Text`. Removed references to the following mixins to use the new `Text` or tokens directly in classes: `text-style-body`, `text-style-heading`, `text-style-subheading`, `text-style-caption`, `text-style-button`, `text-style-button-large`, `text-emphasis-subdued`, `text-emphasis-strong`, `nav-item-text-attributes`. - [#7577](#7577) [`db951f855`](db951f8) Thanks [@RickyMarou](https://github.com/RickyMarou)! - Page component: display subtitle even when it's the only header prop set - [#7633](#7633) [`1364be7f1`](1364be7) Thanks [@kyledurand](https://github.com/kyledurand)! - Renamed `alignY` prop to `alignBlock` on `Inline` Added more flex properties to `align` on `Inline` - [#7443](#7443) [`7a6fb7c1c`](7a6fb7c) Thanks [@iAmNathanJ](https://github.com/iAmNathanJ)! - Improve performance of the Scrollable component with React 18 - [#7625](#7625) [`9f8b651dd`](9f8b651) Thanks [@kyledurand](https://github.com/kyledurand)! - Removed wrap children with div from Inline component - [#7593](#7593) [`addd6bcdd`](addd6bc) Thanks [@kyledurand](https://github.com/kyledurand)! - Improved comments across layout components, changed default spacing of Inline component to match AlphaStack - [#7600](#7600) [`f006509be`](f006509) Thanks [@billycai](https://github.com/billycai)! - Add spacing between title and metadata for Page component - [#7563](#7563) [`a9051d678`](a9051d6) Thanks [@chazdean](https://github.com/chazdean)! - Updated `Inline` component docs and default prop values - [#7635](#7635) [`3cb5377a6`](3cb5377) Thanks [@iAmNathanJ](https://github.com/iAmNathanJ)! - Fixed Scrollable component to match existing onScrolledToBottom logic - Updated dependencies \[[`432bdd5fe`](432bdd5), [`6e9edd3b5`](6e9edd3), [`35be8a003`](35be8a0)]: - @shopify/polaris-icons@6.5.0 - @shopify/polaris-tokens@6.3.0 ## @shopify/polaris-tokens@6.3.0 ### Minor Changes - [#7621](#7621) [`6e9edd3b5`](6e9edd3) Thanks [@aveline](https://github.com/aveline)! - - Added border width prop to `Box` - Exported color token subset alias types from tokens package and remove from `Box` ## @shopify/stylelint-polaris@4.4.0 ### Minor Changes - [#7551](#7551) [`d7dc4436f`](d7dc443) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Add `stylelint-polaris/coverage` rule ### Patch Changes - [#7589](#7589) [`b7b0ef5a9`](b7b0ef5) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Add constraints to `stylelint-polaris/coverage` disable comments - Updated dependencies \[[`6e9edd3b5`](6e9edd3)]: - @shopify/polaris-tokens@6.3.0 ## @shopify/plugin-polaris@0.0.16 ### Patch Changes - Updated dependencies \[[`8859f5db5`](8859f5d), [`cf7badbd1`](cf7badb), [`cf7badbd1`](cf7badb), [`cf7badbd1`](cf7badb), [`8c1989618`](8c19896), [`3652eb901`](3652eb9), [`1f2ec8bfe`](1f2ec8b), [`ba576498d`](ba57649)]: - @shopify/polaris-migrator@0.8.0 ## polaris.shopify.com@0.24.0 ### Minor Changes - [#7068](#7068) [`ccdcea22e`](ccdcea2) Thanks [@laurkim](https://github.com/laurkim)! - Deprecated `DisplayText`, `Heading`, `Subheading`, `Caption`, `TextStyle`, and `VisuallyHidden` pages and removed examples - [#7609](#7609) [`343865159`](3438651) Thanks [@sarahill](https://github.com/sarahill)! - Added new type style guidance and info to typography docs ### Patch Changes - [#7634](#7634) [`4db441756`](4db4417) Thanks [@laurkim](https://github.com/laurkim)! - Replaced usage of typography components (`DisplayText`, `Heading`, `Subheading`, `Caption`, `VisuallyHidden`, `TextStyle`) with the new `Text` component - [#7604](#7604) [`aa82c82ff`](aa82c82) Thanks [@chazdean](https://github.com/chazdean)! - Updated `Inline` component doc vertical alignment example - [#7568](#7568) [`ab0cf251f`](ab0cf25) Thanks [@chazdean](https://github.com/chazdean)! - Updated `AlphaCard` component guidance and examples - [#7633](#7633) [`1364be7f1`](1364be7) Thanks [@kyledurand](https://github.com/kyledurand)! - Renamed `alignY` prop to `alignBlock` on `Inline` Added more flex properties to `align` on `Inline` - [#7527](#7527) [`924e9e5cd`](924e9e5) Thanks [@chazdean](https://github.com/chazdean)! - Update `Columns` component docs - [#7596](#7596) [`749ee31ee`](749ee31) Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed home promo image layout - [#7563](#7563) [`a9051d678`](a9051d6) Thanks [@chazdean](https://github.com/chazdean)! - Updated `Inline` component docs and default prop values - [#7566](#7566) [`567822218`](5678222) Thanks [@kyledurand](https://github.com/kyledurand)! - Bumped nextjs - [#7602](#7602) [`9931ce0b4`](9931ce0) Thanks [@kyledurand](https://github.com/kyledurand)! - Bumped nextjs to 13.0.1 - [#7571](#7571) [`4c5ccc8fa`](4c5ccc8) Thanks [@chazdean](https://github.com/chazdean)! - Updated `Bleed` component guidance and examples - Updated dependencies \[[`b3e73ee04`](b3e73ee), [`20c8cad81`](20c8cad), [`79d92a820`](79d92a8), [`db951f855`](db951f8), [`432bdd5fe`](432bdd5), [`6e9edd3b5`](6e9edd3), [`ccdcea22e`](ccdcea2), [`1364be7f1`](1364be7), [`7a6fb7c1c`](7a6fb7c), [`9f8b651dd`](9f8b651), [`addd6bcdd`](addd6bc), [`f006509be`](f006509), [`a9051d678`](a9051d6), [`3cb5377a6`](3cb5377), [`35be8a003`](35be8a0)]: - @shopify/polaris@10.11.0 - @shopify/polaris-icons@6.5.0 - @shopify/polaris-tokens@6.3.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

iAmNathanJ force-pushed the scrollable-rewrite branch from f37d98b to 56c2ce4 Compare October 18, 2022 23:39

iAmNathanJ force-pushed the scrollable-rewrite branch from 56c2ce4 to 19af0a0 Compare October 18, 2022 23:48

iAmNathanJ changed the base branch from fix-scrollable-jank to main October 18, 2022 23:50

iAmNathanJ mentioned this pull request Oct 19, 2022

[WIP] [Scrollable] Fix scroll jank - React 18 #7425

Closed

9 tasks

iAmNathanJ changed the title ~~[WIP] [Scrollable] refactor: rewrite Scrollable utilizing platform features~~ [Scrollable] refactor: rewrite Scrollable utilizing platform features Oct 19, 2022

iAmNathanJ requested a review from kyledurand October 20, 2022 14:17

kyledurand requested a review from AndrewMusgrave October 20, 2022 17:40

kyledurand reviewed Oct 20, 2022

View reviewed changes

.changeset/many-otters-repeat.md Show resolved Hide resolved

polaris-react/src/components/Scrollable/Scrollable.tsx Outdated Show resolved Hide resolved

polaris-react/src/components/Scrollable/Scrollable.tsx Outdated Show resolved Hide resolved

kyledurand requested a review from tmlayton October 20, 2022 18:02

AndrewMusgrave reviewed Oct 20, 2022

View reviewed changes

iAmNathanJ force-pushed the scrollable-rewrite branch from 54ede17 to bb45daf Compare October 24, 2022 13:55

AndrewMusgrave self-requested a review October 24, 2022 15:47

iAmNathanJ added 3 commits October 24, 2022 13:00

add story for Scrollable using the hint prop

410a38a

Address PR Feedback

332081a

- remove unneeded css class - respect reduced motion settings in scrollTo function - use lazy ref for sticky manager - fix: scroll hint happens at max once per mount - fix: invoke callback when scrolled to bottom of container

iAmNathanJ force-pushed the scrollable-rewrite branch from bb45daf to 332081a Compare October 24, 2022 17:00

tmlayton reviewed Oct 25, 2022

View reviewed changes

polaris-react/src/components/Scrollable/Scrollable.stories.tsx Show resolved Hide resolved

tmlayton reviewed Oct 25, 2022

View reviewed changes

polaris-react/src/components/Scrollable/Scrollable.tsx Show resolved Hide resolved

tmlayton approved these changes Oct 25, 2022

View reviewed changes

iAmNathanJ force-pushed the scrollable-rewrite branch from 69666d5 to 63c2b5b Compare October 25, 2022 17:53

iAmNathanJ added 2 commits October 25, 2022 16:01

add story for onScrollToBottom scenario

d91445f

use requestAnimationFrame to avoid potential reflows in scroll event …

bf52e2e

…handlers

iAmNathanJ force-pushed the scrollable-rewrite branch from 63c2b5b to bf52e2e Compare October 25, 2022 20:02

AndrewMusgrave approved these changes Oct 26, 2022

View reviewed changes

fix: infinite render cycles caused by onScrolledToBottom prop

cd7fc7f

kyledurand reviewed Oct 27, 2022

View reviewed changes

kyledurand reviewed Oct 31, 2022

View reviewed changes

kyledurand approved these changes Oct 31, 2022

View reviewed changes

iAmNathanJ merged commit 7a6fb7c into main Oct 31, 2022

iAmNathanJ deleted the scrollable-rewrite branch October 31, 2022 18:15

github-actions bot mentioned this pull request Oct 31, 2022

Version Packages #7562

Merged

	Improve performance of the Scrollable component with React 18
	Improved performance of the Scrollable component with React 18

[Scrollable] refactor: rewrite Scrollable utilizing platform features #7443

[Scrollable] refactor: rewrite Scrollable utilizing platform features #7443

Uh oh!

Conversation

iAmNathanJ commented Oct 18, 2022 • 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?

Before

After

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

iAmNathanJ commented Oct 19, 2022

Uh oh!

github-actions bot commented Oct 19, 2022

Uh oh!

BPScott commented Oct 20, 2022

Uh oh!

kyledurand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kyledurand commented Oct 21, 2022

Uh oh!

iAmNathanJ commented Oct 21, 2022

Uh oh!

Uh oh!

Uh oh!

tmlayton left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iAmNathanJ commented Oct 27, 2022

Uh oh!

github-actions bot commented Oct 27, 2022

Uh oh!

kyledurand left a comment

Choose a reason for hiding this comment

Uh oh!

kyledurand Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

iAmNathanJ Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iAmNathanJ commented Oct 18, 2022 •

edited

Loading

github-actions bot commented Oct 18, 2022 •

edited

Loading