Portals with provider #3544

dfmcphee · 2020-10-21T18:07:39Z

WHY are these changes introduced?

This is an alternative approach to #3465.

WHAT is this pull request doing?

It changes the portals to use a provider that passes context for the container to render into instead of relying on all the manual DOM manipulation we have currently.

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

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

3tests

github-actions · 2020-10-22T01:01:49Z

🟡 This pull request modifies 16 files and might impact 66 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 66)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AppProvider/AppProvider.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Frame/components/ToastManager/ToastManager.tsx (total: 1)

Files potentially affected (total: 1)

/src/components/Frame/Frame.tsx

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

🧩 src/components/PolarisTestProvider/PolarisTestProvider.tsx (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 60)

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

🧩 src/utilities/portals/PortalsContainer.tsx (total: 64)

Files potentially affected (total: 64)

🧩 src/utilities/portals/PortalsManagerProvider.tsx (total: 64)

Files potentially affected (total: 64)

🧩 src/utilities/portals/context.tsx (total: 66)

Files potentially affected (total: 66)

🧩 src/utilities/portals/hooks.ts (total: 64)

Files potentially affected (total: 64)

🧩 src/utilities/portals/index.ts (total: 63)

Files potentially affected (total: 63)

kyledurand

🎩 and code looks good. This should work really well with #3550 so that nested portals don't render custom properties if they match the parent, now that they're nested within the main theme provider

kyledurand · 2020-10-22T15:36:59Z

UNRELEASED.md

 ### Enhancements

-Updated `Textfield` with a `type` of `number` to not render a spinner if step is set to `0` ([#3477](https://github.com/Shopify/polaris-react/pull/3477))
+- Updated Textfield with a type of number to not render a spinner if step is set to 0 ([#3477](https://github.com/Shopify/polaris-react/pull/3477))


This can be 🔥 right? Should be captured in changelog already

It hasn't been released so it can stay until we release an actual version ( not a beta )

kyledurand · 2020-10-22T15:37:08Z

UNRELEASED.md

 ### Bug fixes

-Fixed `Filters` overflow ([#3532](https://github.com/Shopify/polaris-react/pull/3532))
+- Fixed `Filters` overflow ([#3532](https://github.com/Shopify/polaris-react/pull/3532))


Same with this

kyledurand · 2020-10-22T15:37:44Z

src/components/Modal/tests/Modal.test.tsx

      );

-      expect(modal.find(Portal).prop('idPrefix')).toBe('modal');
+      expect(modal.find(Portal)).toHaveLength(1);


dleroux

I can't tophat in web at the moment because of an issue with my build. But looks good!

MLDimitry · 2020-11-16T15:13:24Z

src/utilities/portals/PortalsContainer.tsx

+
+  const {setContainerNode} = portalsManagerContext;
+
+  return <div id="PolarisPortalsContainer" ref={setContainerNode} />;


Should this always be rendered @alex-page?

I'm working on updating from Polaris 5.8.4 to 5.10.1 https://github.com/Shopify/online-store-web/pull/8774

But seeing this error on buildkite for one of our tests

The component should be returning null if not rendered. But with this Polaris update, Polaris seems to always insert this div into the DOM.

Component being tested https://github.com/Shopify/online-store-web/blob/master/app/sections/Themes/ThemeIndex/ThemeIndex.tsx#L324

Failing test https://github.com/Shopify/online-store-web/blob/master/app/sections/Themes/ThemeIndex/tests/ThemeIndex.test.tsx#L107

Based on skimming this PR quickly, seems like portalsManagerContext would always exist in this component so would never render null. So seems like it is always rendered, but maybe this is necessary for supporting the Modal's/Portal's

Hey @MLDimitry. Yep, you are correct. The way the portals work now this container is always rendered. Your test makes the assumption that none of the providers also output markup (not just your component). I would like to make this more automatic but for now you could update your test to something like expect(wrapper.find(ThemeIndex).html()).toBe(''); to be a bit more specific.

Thanks for confirming what I figured + the context, I'll update the test to be more specific then 👍

Co-authored-by: Daniel Leroux <daniel.leroux@shopify.com>

dleroux and others added 7 commits October 21, 2020 20:53

adding single portal container

154fe05

3tests

change log

6be9921

Refactoring portal to be a functional component

2afe306

Fix cleanup method

b9126f8

Refactor to use PortalsManagerProvider

8d30aba

Fixing tests

c703575

Updating UNRELEASED log

1d8c2a4

dfmcphee force-pushed the portals-with-provider branch from 9e0a170 to 1d8c2a4 Compare October 22, 2020 00:59

dfmcphee marked this pull request as ready for review October 22, 2020 02:28

dfmcphee requested review from alex-page, dleroux and kyledurand October 22, 2020 13:35

alex-page approved these changes Oct 22, 2020

View reviewed changes

alex-page mentioned this pull request Oct 22, 2020

[Portal] Render single div for container element #3465

Closed

6 tasks

kyledurand approved these changes Oct 22, 2020

View reviewed changes

dleroux approved these changes Oct 22, 2020

View reviewed changes

alex-page merged commit 5b08e9f into master Oct 23, 2020

alex-page deleted the portals-with-provider branch October 23, 2020 12:40

MLDimitry reviewed Nov 16, 2020

View reviewed changes

sylvhama pushed a commit that referenced this pull request Mar 26, 2021

Portals with provider (#3544)

dfd9365

Co-authored-by: Daniel Leroux <daniel.leroux@shopify.com>


		const {setContainerNode} = portalsManagerContext;

		return <div id="PolarisPortalsContainer" ref={setContainerNode} />;

Portals with provider #3544

Portals with provider #3544

Uh oh!

Conversation

dfmcphee commented Oct 21, 2020 • 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?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Oct 22, 2020

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 60)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 64)

Files potentially affected (total: 64)

Files potentially affected (total: 66)

Files potentially affected (total: 64)

Files potentially affected (total: 63)

Uh oh!

kyledurand left a comment

Choose a reason for hiding this comment

Uh oh!

kyledurand Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

alex-page Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

kyledurand Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

kyledurand Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

dleroux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MLDimitry Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfmcphee Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

MLDimitry Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dfmcphee commented Oct 21, 2020 •

edited

Loading

dleroux left a comment •

edited

Loading

MLDimitry Nov 16, 2020 •

edited

Loading