Skip to content

Conversation

iAmNathanJ
Copy link
Contributor

@iAmNathanJ iAmNathanJ commented Oct 17, 2022

WHY are these changes introduced?

Fixes #7424

Addresses scrolling performance in <Scrollable> that relates to automatic batching in React.

WHAT is this pull request doing?

This is a WIP PR to discuss fixes for the scrolling problem explained in #7424

  • Disables box shadow on scroll containers to eliminate the question of box shadow performance.

Option 1:

  • Uses ReactDOM.flushSync to apply state updates internal the <Scrollable>. I'm guessing this isn't a realistic solution since some consumers could still be on React 17 or 16
  • Refines the existing behavior to set scrollTop on the scroll container. This certainly improves the UX for scrolling in general use cases, but I'm unsure if it would cause unwanted behavior.

Option 2:

  • could we just remove the entire componentDidUpdate? Again, I'm unsure if it would cause unwanted behavior, but it would mean we don't need to use flushSync. Reading the component more thoroughly now, it looks like we def need the componentDidUpdate since there is a controlled aspect to the scroll when using scrollTo and scroll hint. Those could possibly be reworked to just use platform features and not act as side effects of state updates. 🤔

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2022

size-limit report 📦

Path Size
polaris-react-cjs 209.18 KB (+0.03% 🔺)
polaris-react-esm 135.64 KB (+0.03% 🔺)
polaris-react-esnext 190.99 KB (+0.02% 🔺)
polaris-react-css 41.51 KB (0%)

@iAmNathanJ
Copy link
Contributor Author

Closing in favor of #7443

@iAmNathanJ iAmNathanJ closed this Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrollable broken with React 18 automatic batching
1 participant