Skip to content

Conversation

@pt8o
Copy link
Contributor

@pt8o pt8o commented Apr 29, 2021

WHY are these changes introduced?

Needed to fix https://github.com/Shopify/core-issues/issues/23913

We're building a component that has its own sticky toolbar header, and then the main body content is wrapped in a <ScrollableContainer>, so that's how the user scrolls through the modal contents. The problem is, if the browser window is short enough, it'll squish the modal contents such that an outer scrollbar appears, which belongs to Polaris's <Modal>. So we have an outer container and an internal container, each of which will only partially scroll the modal contents 😬

You can see a video in the above issue.

WHAT is this pull request doing?

<Modal> component now has the prop noScroll which when true wraps the body content in a div instead of the Scrollable component

🎩 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
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@ghost
Copy link

ghost commented Apr 29, 2021

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2021

🟢 This pull request modifies 3 files and might impact 3 other files.

Details:
All files potentially affected (total: 3)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Modal/Modal.tsx (total: 3)

Files potentially affected (total: 3)

🧩 src/components/Modal/tests/Modal.test.tsx (total: 0)

Files potentially affected (total: 0)

@pt8o pt8o requested a review from WillsonSmith April 29, 2021 21:04
.Scrollable {
-webkit-overflow-scrolling: touch;
position: relative;
overflow: hidden;
Copy link
Contributor

@WillsonSmith WillsonSmith Apr 30, 2021

Choose a reason for hiding this comment

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

I think we should just not render the Scrollable in the Modal when noScroll is present. Rather than changing the Scrollable in addition to the modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call, I was overthinking it. Scrollable has a shadow prop that makes some styles that I didn't want to repeat, but obviously if the content isn't scrollable then there's no need for a shadow.

@pt8o pt8o requested a review from WillsonSmith April 30, 2021 15:19
display: flex;
flex-grow: 1;
overflow-x: hidden;
overflow: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this need to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, leftover change from my prev idea

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@pt8o pt8o force-pushed the modal-noscroll branch from f35fe5e to 977cf33 Compare May 5, 2021 17:49
@pt8o pt8o force-pushed the modal-noscroll branch from 977cf33 to f263d47 Compare May 5, 2021 17:51
@alex-page alex-page merged commit 90677db into main May 6, 2021
@alex-page alex-page deleted the modal-noscroll branch May 6, 2021 18:38
@ghost
Copy link

ghost commented May 6, 2021

🎉 Thanks for your contribution to Polaris React!

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.

4 participants