Skip to content
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.

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

[Popover] fix scrolling position reseted on re-render #11961

Closed
wants to merge 1 commit into from

Conversation

willnguyen1312
Copy link
Member

@willnguyen1312 willnguyen1312 commented Apr 30, 2024

WHY are these changes introduced?

Fixes #11960

WHAT is this pull request doing?

As Popover component re-renders which cause the desiredHeight reseted to 0 and back to expected value. This seems to work fine if the popover's content is small without overflow. Yet, it creates an issue when there is a scrolling position in place. We can fix it by storing the last computed value and reuse it in order to avoid scrolling position resettled to 0

How to 🎩

Set up storybook as per this sandbox - https://codesandbox.io/p/sandbox/amazing-paper-qrv23n

Open the sample popover component, then scroll to bottom and click on the bottom button to re-render the component. The scroll position should remain intact without being reseted to 0

Before:

Popover.Before.mov

After:

Popover.After.mov

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@willnguyen1312 willnguyen1312 changed the title [Popover] fix scrolling position resetted on re-render [Popover] fix scrolling position reseted on re-render Apr 30, 2024
@willnguyen1312 willnguyen1312 force-pushed the fix-popover-losing-scroll-position branch from cbbe8ba to 2c2f3c2 Compare April 30, 2024 21:47
@willnguyen1312 willnguyen1312 marked this pull request as ready for review May 1, 2024 11:48
@willnguyen1312
Copy link
Member Author

/snapit

Copy link
Contributor

github-actions bot commented May 1, 2024

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

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

"@shopify/polaris-migrator": "0.0.0-snapshot-20240501141452",
"@shopify/polaris": "0.0.0-snapshot-20240501141452",
"@shopify/polaris-tokens": "0.0.0-snapshot-20240501141452",
"@shopify/stylelint-polaris": "0.0.0-snapshot-20240501141452"

@willnguyen1312
Copy link
Member Author

Sorry, closed as this solution can fix the scrolling issue yet would break dynamic children content. Due to the fact that the desired height will be recalculated upon content changes, we need to reset the height to auto in order to compute it correctly and return back the correct height 😅

@willnguyen1312 willnguyen1312 deleted the fix-popover-losing-scroll-position branch May 1, 2024 16:38
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.

Popover component lost scroll position on re-render
1 participant