From 8ac4d074b9a8270f1a14bfe4dc05d1b0a1fe763b Mon Sep 17 00:00:00 2001 From: Daniel Leroux Date: Tue, 29 Sep 2020 18:34:31 -0400 Subject: [PATCH 1/5] adding single portal container 3tests --- .../Pagination/tests/Pagination.test.tsx | 31 ++++----- src/components/Portal/Portal.tsx | 41 +++++++++--- src/components/Portal/tests/Portal.test.tsx | 63 ++++++++++++------- src/components/Tabs/tests/Tabs.test.tsx | 7 +++ 4 files changed, 95 insertions(+), 47 deletions(-) diff --git a/src/components/Pagination/tests/Pagination.test.tsx b/src/components/Pagination/tests/Pagination.test.tsx index 477f7b22941..478b2c14d61 100644 --- a/src/components/Pagination/tests/Pagination.test.tsx +++ b/src/components/Pagination/tests/Pagination.test.tsx @@ -1,10 +1,6 @@ import React from 'react'; // eslint-disable-next-line no-restricted-imports -import { - mountWithAppProvider, - findByTestID, - ReactWrapper, -} from 'test-utilities/legacy'; +import {mountWithAppProvider, ReactWrapper} from 'test-utilities/legacy'; import {mountWithApp} from 'test-utilities'; import {Tooltip, TextField} from 'components'; @@ -18,6 +14,13 @@ interface HandlerMap { [eventName: string]: (event: any) => void; } +jest.mock('../../Portal', () => ({ + ...(jest.requireActual('../../Portal') as any), + Portal() { + return null; + }, +})); + const listenerMap: HandlerMap = {}; describe('', () => { @@ -47,12 +50,10 @@ describe('', () => { describe('tooltip', () => { it('renders a tooltip if nextTooltip is provided and hasNext is true', () => { - const pagination = mountWithAppProvider( - , - ); - pagination.find(Tooltip).simulate('focus'); - - expect(findByTestID(pagination, 'TooltipOverlayLabel').text()).toBe('k'); + const pagination = mountWithApp(); + expect(pagination).toContainReactComponent(Tooltip, { + content: 'k', + }); }); it('does not render a tooltip if nextTooltip is provided and hasNext is false', () => { @@ -63,12 +64,12 @@ describe('', () => { }); it('renders a tooltip if previousToolTip is provided and hasPrevious is true', () => { - const pagination = mountWithAppProvider( + const pagination = mountWithApp( , ); - pagination.find(Tooltip).simulate('focus'); - - expect(findByTestID(pagination, 'TooltipOverlayLabel').text()).toBe('j'); + expect(pagination).toContainReactComponent(Tooltip, { + content: 'j', + }); }); it('does not render tooltip if previousToolTip is provided and hasPrevious is false', () => { diff --git a/src/components/Portal/Portal.tsx b/src/components/Portal/Portal.tsx index 2d97dfe5481..4244659e9cd 100644 --- a/src/components/Portal/Portal.tsx +++ b/src/components/Portal/Portal.tsx @@ -15,6 +15,8 @@ interface State { isMounted: boolean; } +export const UNIQUE_CONTAINER_ID = 'polaris-portal-container'; + const getUniqueID = globalIdGeneratorFactory('portal-'); export class Portal extends PureComponent { @@ -26,37 +28,53 @@ export class Portal extends PureComponent { private portalNode: HTMLElement | null = null; + private portalContainerNode: HTMLElement | null = null; + private portalId = this.props.idPrefix !== '' ? `${this.props.idPrefix}-${getUniqueID()}` : getUniqueID(); componentDidMount() { + const constainerNode = document.getElementById(UNIQUE_CONTAINER_ID); + + this.portalContainerNode = constainerNode || document.createElement('div'); + + if (!constainerNode) + this.portalContainerNode.setAttribute('id', UNIQUE_CONTAINER_ID); this.portalNode = document.createElement('div'); this.portalNode.setAttribute(portal.props[0], this.portalId); - if (this.context != null) { + if (this.context != null && !constainerNode) { const {cssCustomProperties} = this.context; if (cssCustomProperties != null) { - this.portalNode.setAttribute('style', cssCustomProperties); + this.portalContainerNode.setAttribute('style', cssCustomProperties); } else { - this.portalNode.removeAttribute('style'); + this.portalContainerNode.removeAttribute('style'); } } - document.body.appendChild(this.portalNode); + + if (!constainerNode) { + document.body.appendChild(this.portalContainerNode); + } + + this.portalContainerNode.appendChild(this.portalNode); + this.setState({isMounted: true}); } componentDidUpdate(_: PortalProps, prevState: State) { const {onPortalCreated = noop} = this.props; - if (this.portalNode && this.context != null) { + if (this.portalContainerNode && this.context != null) { const {cssCustomProperties, textColor} = this.context; if (cssCustomProperties != null) { - const style = `${cssCustomProperties};color:${textColor};`; - this.portalNode.setAttribute('style', style); + const style = textColor + ? `${cssCustomProperties};color:${textColor};` + : `${cssCustomProperties}`; + this.portalContainerNode.setAttribute('style', style); } else { - this.portalNode.removeAttribute('style'); + this.portalContainerNode.removeAttribute('style'); } } if (!prevState.isMounted && this.state.isMounted) { @@ -65,8 +83,11 @@ export class Portal extends PureComponent { } componentWillUnmount() { - if (this.portalNode) { - document.body.removeChild(this.portalNode); + if (this.portalNode && this.portalContainerNode) { + this.portalContainerNode.removeChild(this.portalNode); + if (this.portalContainerNode.childElementCount === 0) { + document.body.removeChild(this.portalContainerNode); + } } } diff --git a/src/components/Portal/tests/Portal.test.tsx b/src/components/Portal/tests/Portal.test.tsx index 7c71463d0d6..212716017de 100644 --- a/src/components/Portal/tests/Portal.test.tsx +++ b/src/components/Portal/tests/Portal.test.tsx @@ -1,9 +1,9 @@ import React, {createRef} from 'react'; -import {mount} from 'test-utilities'; +import {mount, mountWithApp} from 'test-utilities'; // eslint-disable-next-line no-restricted-imports import {mountWithAppProvider} from 'test-utilities/legacy'; -import {Portal} from '../Portal'; +import {Portal, UNIQUE_CONTAINER_ID} from '../Portal'; import {portal} from '../../shared'; jest.mock('react-dom', () => ({ @@ -24,6 +24,45 @@ describe('', () => { createPortalSpy.mockImplementation(() => null); }); + afterEach(() => { + createPortalSpy.mockRestore(); + }); + + describe('container', () => { + it('creates a portal container with the UNIQUE_CONTAINER_ID', () => { + const appendChildSpy = jest.spyOn(document.body, 'appendChild'); + mountWithApp(); + expect(appendChildSpy).toHaveBeenLastCalledWith( + expect.objectContaining({ + id: UNIQUE_CONTAINER_ID, + }), + ); + appendChildSpy.mockReset(); + }); + + it('sets CSS custom properties on the portal node', () => { + const setSpy = jest.spyOn(Element.prototype, 'setAttribute'); + const portal = mountWithAppProvider(, { + features: {newDesignLanguage: true}, + theme: { + colors: {surface: '#000000'}, + }, + }); + + expect(setSpy).toHaveBeenLastCalledWith( + 'style', + portal.context().cssCustomProperties, + ); + setSpy.mockRestore(); + }); + + it('removes CSS custom properties from the portal node', () => { + const removeSpy = jest.spyOn(Element.prototype, 'removeAttribute'); + mountWithAppProvider(); + expect(removeSpy).toHaveBeenCalledWith('style'); + }); + }); + describe('children', () => { it('get passed into the portal creation method', () => { const children =
; @@ -100,24 +139,4 @@ describe('', () => { mount(); }).not.toThrow(); }); - - it('sets CSS custom properties on the portal node', () => { - const setSpy = jest.spyOn(Element.prototype, 'setAttribute'); - const portal = mountWithAppProvider(, { - features: {newDesignLanguage: true}, - theme: { - colors: {surface: '#000000'}, - }, - }); - expect(setSpy).toHaveBeenCalledWith( - 'style', - portal.context().cssCustomProperties, - ); - }); - - it('removes CSS custom properties from the portal node', () => { - const removeSpy = jest.spyOn(Element.prototype, 'removeAttribute'); - mountWithAppProvider(); - expect(removeSpy).toHaveBeenCalledWith('style'); - }); }); diff --git a/src/components/Tabs/tests/Tabs.test.tsx b/src/components/Tabs/tests/Tabs.test.tsx index a26fa3c1213..577eee43690 100644 --- a/src/components/Tabs/tests/Tabs.test.tsx +++ b/src/components/Tabs/tests/Tabs.test.tsx @@ -9,6 +9,13 @@ import {getVisibleAndHiddenTabIndices} from '../utilities'; import {FeaturesContext} from '../../../utilities/features'; import {Popover} from '../../Popover'; +jest.mock('../../Portal', () => ({ + ...(jest.requireActual('../../Portal') as any), + Portal() { + return null; + }, +})); + describe('', () => { const tabs: TabsProps['tabs'] = [ {content: 'Tab 1', id: 'tab-1'}, From ac3955a8c0854fb863827c37a36e5654cbf9262a Mon Sep 17 00:00:00 2001 From: Daniel Leroux Date: Sun, 18 Oct 2020 08:19:27 -0400 Subject: [PATCH 2/5] change log --- UNRELEASED.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/UNRELEASED.md b/UNRELEASED.md index fa215e7a761..cb4ed7eb5e2 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -8,6 +8,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Enhancements +- Changed `Portal` to render all `Portals` in a single div ([#3465](https://github.com/Shopify/polaris-react/pull/3465)) + ### Bug fixes ### Documentation From f35381c27e607c553a192ce0a665fcded56244ac Mon Sep 17 00:00:00 2001 From: Dominic McPhee Date: Tue, 20 Oct 2020 17:49:54 -0400 Subject: [PATCH 3/5] Refactoring portal to be a functional component --- src/components/Portal/Portal.tsx | 110 +++++++++++++++---------------- 1 file changed, 53 insertions(+), 57 deletions(-) diff --git a/src/components/Portal/Portal.tsx b/src/components/Portal/Portal.tsx index 4244659e9cd..9012e4f56d0 100644 --- a/src/components/Portal/Portal.tsx +++ b/src/components/Portal/Portal.tsx @@ -1,4 +1,4 @@ -import React, {PureComponent} from 'react'; +import React, {useState, useRef, useContext, useEffect} from 'react'; import {createPortal} from 'react-dom'; import {ThemeContext} from '../../utilities/theme'; @@ -11,91 +11,87 @@ export interface PortalProps { onPortalCreated?(): void; } -interface State { - isMounted: boolean; -} - export const UNIQUE_CONTAINER_ID = 'polaris-portal-container'; const getUniqueID = globalIdGeneratorFactory('portal-'); -export class Portal extends PureComponent { - static defaultProps = {idPrefix: ''}; - static contextType = ThemeContext; - context!: React.ContextType; - - state: State = {isMounted: false}; - - private portalNode: HTMLElement | null = null; - - private portalContainerNode: HTMLElement | null = null; +export function Portal({ + children, + idPrefix = '', + onPortalCreated = noop, +}: PortalProps) { + const [isMounted, setIsMounted] = useState(false); + const portalNode = useRef(null); + const portalsContainerNode = useRef(null); + const context = useContext(ThemeContext); - private portalId = - this.props.idPrefix !== '' - ? `${this.props.idPrefix}-${getUniqueID()}` - : getUniqueID(); + const portalId = + idPrefix !== '' ? `${idPrefix}-${getUniqueID()}` : getUniqueID(); - componentDidMount() { - const constainerNode = document.getElementById(UNIQUE_CONTAINER_ID); + useEffect(() => { + const containerNode = document.getElementById(UNIQUE_CONTAINER_ID); - this.portalContainerNode = constainerNode || document.createElement('div'); + portalsContainerNode.current = + containerNode || document.createElement('div'); - if (!constainerNode) - this.portalContainerNode.setAttribute('id', UNIQUE_CONTAINER_ID); - this.portalNode = document.createElement('div'); - this.portalNode.setAttribute(portal.props[0], this.portalId); + if (!containerNode) + portalsContainerNode.current.setAttribute('id', UNIQUE_CONTAINER_ID); + portalNode.current = document.createElement('div'); + portalNode.current.setAttribute(portal.props[0], portalId); - if (this.context != null && !constainerNode) { - const {cssCustomProperties} = this.context; + if (context != null && !containerNode) { + const {cssCustomProperties} = context; if (cssCustomProperties != null) { - this.portalContainerNode.setAttribute('style', cssCustomProperties); + portalsContainerNode.current.setAttribute('style', cssCustomProperties); } else { - this.portalContainerNode.removeAttribute('style'); + portalsContainerNode.current.removeAttribute('style'); } } - if (!constainerNode) { - document.body.appendChild(this.portalContainerNode); + if (!containerNode) { + document.body.appendChild(portalsContainerNode.current); } - this.portalContainerNode.appendChild(this.portalNode); + portalsContainerNode.current.appendChild(portalNode.current); - this.setState({isMounted: true}); - } + setIsMounted(true); + }, [isMounted, context, portalId]); - componentDidUpdate(_: PortalProps, prevState: State) { - const {onPortalCreated = noop} = this.props; - - if (this.portalContainerNode && this.context != null) { - const {cssCustomProperties, textColor} = this.context; + useEffect(() => { + if (isMounted && portalsContainerNode.current && context != null) { + const {cssCustomProperties, textColor} = context; if (cssCustomProperties != null) { const style = textColor ? `${cssCustomProperties};color:${textColor};` : `${cssCustomProperties}`; - this.portalContainerNode.setAttribute('style', style); + const currentStyle = portalsContainerNode.current.getAttribute('style'); + if (style !== currentStyle) { + portalsContainerNode.current.setAttribute('style', style); + } } else { - this.portalContainerNode.removeAttribute('style'); + portalsContainerNode.current.removeAttribute('style'); } } - if (!prevState.isMounted && this.state.isMounted) { - onPortalCreated(); - } - } - componentWillUnmount() { - if (this.portalNode && this.portalContainerNode) { - this.portalContainerNode.removeChild(this.portalNode); - if (this.portalContainerNode.childElementCount === 0) { - document.body.removeChild(this.portalContainerNode); + return function cleanup() { + if (portalNode.current && portalsContainerNode.current) { + portalsContainerNode.current.removeChild(portalNode.current); + if (portalsContainerNode.current.childElementCount === 0) { + document.body.removeChild(portalsContainerNode.current); + } } + }; + }, [isMounted, context]); + + useEffect(() => { + if (isMounted) { + onPortalCreated(); } - } + }, [isMounted, onPortalCreated]); - render() { - return this.portalNode && this.state.isMounted - ? createPortal(this.props.children, this.portalNode) - : null; - } + return portalNode.current && isMounted + ? createPortal(children, portalNode.current) + : null; } function noop() {} From 283decbefaab8f6b4c39558310f681ac03c292ff Mon Sep 17 00:00:00 2001 From: Dominic McPhee Date: Wed, 21 Oct 2020 11:22:24 -0400 Subject: [PATCH 4/5] Fix cleanup method --- src/components/Portal/Portal.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/components/Portal/Portal.tsx b/src/components/Portal/Portal.tsx index 9012e4f56d0..6daa86608b3 100644 --- a/src/components/Portal/Portal.tsx +++ b/src/components/Portal/Portal.tsx @@ -74,10 +74,13 @@ export function Portal({ } return function cleanup() { - if (portalNode.current && portalsContainerNode.current) { - portalsContainerNode.current.removeChild(portalNode.current); - if (portalsContainerNode.current.childElementCount === 0) { - document.body.removeChild(portalsContainerNode.current); + const containerNode = portalsContainerNode.current + ? portalsContainerNode.current + : document.getElementById(UNIQUE_CONTAINER_ID); + if (portalNode.current && containerNode) { + containerNode.removeChild(portalNode.current); + if (containerNode && containerNode.childElementCount === 0) { + document.body.removeChild(containerNode); } } }; From 0381cab955c07b5be819685169f51745f421d8d5 Mon Sep 17 00:00:00 2001 From: Daniel Leroux Date: Wed, 21 Oct 2020 11:36:29 -0400 Subject: [PATCH 5/5] Update UNRELEASED.md --- UNRELEASED.md | 1 + 1 file changed, 1 insertion(+) diff --git a/UNRELEASED.md b/UNRELEASED.md index ae6ae1f2866..d191bcd4836 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -8,6 +8,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Enhancements +- Converted `Portal` to a functional component ([#3465](https://github.com/Shopify/polaris-react/pull/3465)) - Changed `Portal` to render all `Portals` in a single div ([#3465](https://github.com/Shopify/polaris-react/pull/3465)) - 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))