Skip to content

Conversation

@danrosenthal
Copy link

Co-authored-by: Tim Layton tim.layton@shopify.com

WHY are these changes introduced?

Currently (in master) the Portal component renders portals directly within the body element. This places any components rendered in a Portal (Modal, Sheet) outside the scope of the ThemeProvider node. The theme provider component sets its custom properties on a node, so in order to make the modal and sheet components theme-able with the the theme provider component, they need to be rendered within the theme provider component's node.

WHAT is this pull request doing?

This pull request adds a way to hook into the theme provider component's node, and moves the portal component's node within that node.

How to 🎩

Use the chromaui build to tophat z-index and anything else you can think of on the examples for Frame, Modal, Sheet, and Filters.

@danrosenthal danrosenthal force-pushed the portal-within-theme-provider-node branch from a533026 to b10fda1 Compare October 3, 2019 12:12
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2019

Results

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified5
Files potentially affected97

Details

All files potentially affected (total: 97)
UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Portal/Portal.tsx (total: 33)

Files potentially affected (total: 33)

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

Files potentially affected (total: 0)

🧩 src/components/ThemeProvider/ThemeProvider.tsx (total: 1)

Files potentially affected (total: 1)

🧩 src/components/shared.ts (total: 97)

Files potentially affected (total: 97)


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

Co-authored-by: Tim Layton <tim.layton@shopify.com>
@danrosenthal danrosenthal force-pushed the portal-within-theme-provider-node branch from 4d076ca to d62cff6 Compare October 3, 2019 17:55
@danrosenthal
Copy link
Author

Codecov checks have been slow today, so please don't wait for that check to clear before you review @BPScott @tmlayton @dleroux

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

👍💖

Co-Authored-By: Tim Layton <tmlayton@users.noreply.github.com>
@danrosenthal
Copy link
Author

Gonna probably merge this without codecov as well. Any last words @BPScott or @dleroux?

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.

2 participants