diff --git a/UNRELEASED.md b/UNRELEASED.md index 0b000825b22..d191bcd4836 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -8,7 +8,9 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### 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)) +- 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)) ### Bug fixes 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..6daa86608b3 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,70 +11,90 @@ 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; +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); - state: State = {isMounted: false}; + const portalId = + idPrefix !== '' ? `${idPrefix}-${getUniqueID()}` : getUniqueID(); - private portalNode: HTMLElement | null = null; + useEffect(() => { + const containerNode = document.getElementById(UNIQUE_CONTAINER_ID); - private portalId = - this.props.idPrefix !== '' - ? `${this.props.idPrefix}-${getUniqueID()}` - : getUniqueID(); + portalsContainerNode.current = + containerNode || document.createElement('div'); - componentDidMount() { - 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) { - const {cssCustomProperties} = this.context; + if (context != null && !containerNode) { + const {cssCustomProperties} = context; if (cssCustomProperties != null) { - this.portalNode.setAttribute('style', cssCustomProperties); + portalsContainerNode.current.setAttribute('style', cssCustomProperties); } else { - this.portalNode.removeAttribute('style'); + portalsContainerNode.current.removeAttribute('style'); } } - document.body.appendChild(this.portalNode); - this.setState({isMounted: true}); - } - componentDidUpdate(_: PortalProps, prevState: State) { - const {onPortalCreated = noop} = this.props; + if (!containerNode) { + document.body.appendChild(portalsContainerNode.current); + } + + portalsContainerNode.current.appendChild(portalNode.current); - if (this.portalNode && this.context != null) { - const {cssCustomProperties, textColor} = this.context; + setIsMounted(true); + }, [isMounted, context, portalId]); + + useEffect(() => { + if (isMounted && portalsContainerNode.current && context != null) { + const {cssCustomProperties, textColor} = context; if (cssCustomProperties != null) { - const style = `${cssCustomProperties};color:${textColor};`; - this.portalNode.setAttribute('style', style); + const style = textColor + ? `${cssCustomProperties};color:${textColor};` + : `${cssCustomProperties}`; + const currentStyle = portalsContainerNode.current.getAttribute('style'); + if (style !== currentStyle) { + portalsContainerNode.current.setAttribute('style', style); + } } else { - this.portalNode.removeAttribute('style'); + portalsContainerNode.current.removeAttribute('style'); } } - if (!prevState.isMounted && this.state.isMounted) { - onPortalCreated(); - } - } - componentWillUnmount() { - if (this.portalNode) { - document.body.removeChild(this.portalNode); + return function cleanup() { + 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); + } + } + }; + }, [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() {} 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'},