From d5c9d6d1f95bc7f981f79864cb0925ce2b31088c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 12:18:12 +0000 Subject: [PATCH 01/23] Fix removeWithinBreakpointListener. It wasn't removing listeners properly, causing components to be updated after unmounting. --- client/lib/viewport/index.js | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/client/lib/viewport/index.js b/client/lib/viewport/index.js index 8f7f4976ad9285..671bce41ed07ac 100644 --- a/client/lib/viewport/index.js +++ b/client/lib/viewport/index.js @@ -41,8 +41,8 @@ // use 769, which is just above the general maximum mobile screen width. const SERVER_WIDTH = 769; -const MOBILE_BREAKPOINT = '<480px'; -const DESKTOP_BREAKPOINT = '>960px'; +export const MOBILE_BREAKPOINT = '<480px'; +export const DESKTOP_BREAKPOINT = '>960px'; const isServer = typeof window === 'undefined' || ! window.matchMedia; @@ -88,6 +88,8 @@ const mediaQueryLists = { '480px-960px': createMediaQueryList( { min: 480, max: 960 } ), }; +const listeners = {}; + function getMediaQueryList( breakpoint ) { if ( ! mediaQueryLists.hasOwnProperty( breakpoint ) ) { try { @@ -106,18 +108,28 @@ export function isWithinBreakpoint( breakpoint ) { } export function addWithinBreakpointListener( breakpoint, listener ) { + if ( ! listener ) { + return; + } + const mediaQueryList = getMediaQueryList( breakpoint ); - if ( mediaQueryList && ! isServer ) { - mediaQueryList.addListener( evt => listener( evt.matches ) ); + if ( mediaQueryList && ! isServer && ! listeners[ listener ] ) { + listeners[ listener ] = evt => listener( evt.matches ); + mediaQueryList.addListener( listeners[ listener ] ); } } export function removeWithinBreakpointListener( breakpoint, listener ) { + if ( ! listener ) { + return; + } + const mediaQueryList = getMediaQueryList( breakpoint ); - if ( mediaQueryList && ! isServer ) { - mediaQueryList.removeListener( listener ); + if ( mediaQueryList && ! isServer && listeners[ listener ] ) { + mediaQueryList.removeListener( listeners[ listener ] ); + delete listeners[ listener ]; } } From add4f108c2888e7d419b36ec9abf4f1375584331 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 12:18:31 +0000 Subject: [PATCH 02/23] Add tests for lib/viewport. --- client/lib/viewport/test/index.js | 227 ++++++++++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 client/lib/viewport/test/index.js diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js new file mode 100644 index 00000000000000..19c0cf8dc5598c --- /dev/null +++ b/client/lib/viewport/test/index.js @@ -0,0 +1,227 @@ +/** + * @format + * @jest-environment jsdom + */ + +/** + * Internal dependencies + */ +let viewport; + +const matchesMock = jest.fn( () => 'foo' ); +const addListenerMock = jest.fn(); +const removeListenerMock = jest.fn(); + +const matchMediaMock = jest.fn( query => { + const mediaListObjectMock = { + addListener: listener => addListenerMock( query, listener ), + removeListener: listener => removeListenerMock( query, listener ), + }; + // Add matches read-only property. + Object.defineProperty( mediaListObjectMock, 'matches', { + get: () => matchesMock( query ), + } ); + return mediaListObjectMock; +} ); + +describe( 'viewport', () => { + beforeAll( async () => { + window.matchMedia = matchMediaMock; + viewport = await import( '..' ); + } ); + + beforeEach( () => { + matchesMock.mockClear(); + addListenerMock.mockClear(); + removeListenerMock.mockClear(); + } ); + + describe( 'isWithinBreakpoint', () => { + test( 'should return undefined when called with no breakpoint', () => { + expect( viewport.isWithinBreakpoint() ).toBe( undefined ); + } ); + + test( 'should return undefined for an unknown breakpoint', () => { + expect( viewport.isWithinBreakpoint( 'unknown' ) ).toBe( undefined ); + } ); + + test( 'should retrieve the current status for a known breakpoint', () => { + expect( viewport.isWithinBreakpoint( '<960px' ) ).toBe( 'foo' ); + expect( matchesMock ).toHaveBeenCalledTimes( 1 ); + expect( matchesMock ).toHaveBeenCalledWith( '(max-width: 960px)' ); + } ); + } ); + + describe( 'isMobile', () => { + test( 'should retrieve the current status for the mobile breakpoint', () => { + expect( viewport.isMobile() ).toBe( 'foo' ); + expect( matchesMock ).toHaveBeenCalledTimes( 1 ); + expect( matchesMock ).toHaveBeenCalledWith( '(max-width: 480px)' ); + } ); + } ); + + describe( 'isDesktop', () => { + test( 'should retrieve the current status for the desktop breakpoint', () => { + expect( viewport.isDesktop() ).toBe( 'foo' ); + expect( matchesMock ).toHaveBeenCalledTimes( 1 ); + expect( matchesMock ).toHaveBeenCalledWith( '(min-width: 961px)' ); + } ); + } ); + + describe( 'addWithinBreakpointListener', () => { + test( 'should do nothing if nothing is provided', () => { + expect( () => viewport.addWithinBreakpointListener() ).not.toThrow(); + expect( addListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should do nothing if an invalid breakpoint is provided', () => { + expect( () => viewport.addWithinBreakpointListener( 'unknown', () => '' ) ).not.toThrow(); + expect( addListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should do nothing if no listener is provided', () => { + expect( () => viewport.addWithinBreakpointListener( '<960px' ) ).not.toThrow(); + expect( addListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should add a listener for a valid breakpoint', () => { + const listener = jest.fn(); + const event = { matches: 'bar' }; + expect( () => viewport.addWithinBreakpointListener( '<960px', listener ) ).not.toThrow(); + expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( addListenerMock ).toHaveBeenCalledWith( + '(max-width: 960px)', + expect.any( Function ) + ); + // Call registered listener to make sure it got added correctly. + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]( event ); + expect( listener ).toHaveBeenCalledTimes( 1 ); + expect( listener ).toHaveBeenCalledWith( 'bar' ); + // Clean up. + try { + viewport.removeWithinBreakpointListener( '<960px', listener ); + } catch {} + } ); + } ); + + describe( 'addIsMobileListener', () => { + test( 'should do nothing if nothing is provided', () => { + expect( () => viewport.addIsMobileListener() ).not.toThrow(); + expect( addListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should add a listener', () => { + const listener = jest.fn(); + const event = { matches: 'bar' }; + expect( () => viewport.addIsMobileListener( listener ) ).not.toThrow(); + expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( addListenerMock ).toHaveBeenCalledWith( + '(max-width: 480px)', + expect.any( Function ) + ); + // Call registered listener to make sure it got added correctly. + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]( event ); + expect( listener ).toHaveBeenCalledTimes( 1 ); + expect( listener ).toHaveBeenCalledWith( 'bar' ); + // Clean up. + try { + viewport.removeIsMobileListener( listener ); + } catch {} + } ); + } ); + + describe( 'addIsDesktopListener', () => { + test( 'should do nothing if nothing is provided', () => { + expect( () => viewport.addIsDesktopListener() ).not.toThrow(); + expect( addListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should add a listener', () => { + const listener = jest.fn(); + const event = { matches: 'bar' }; + expect( () => viewport.addIsDesktopListener( listener ) ).not.toThrow(); + expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( addListenerMock ).toHaveBeenCalledWith( + '(min-width: 961px)', + expect.any( Function ) + ); + // Call registered listener to make sure it got added correctly. + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]( event ); + expect( listener ).toHaveBeenCalledTimes( 1 ); + expect( listener ).toHaveBeenCalledWith( 'bar' ); + // Clean up. + try { + viewport.removeIsDesktopListener( listener ); + } catch {} + } ); + } ); + + describe( 'removeWithinBreakpointListener', () => { + test( 'should do nothing if nothing is provided', () => { + expect( () => viewport.removeWithinBreakpointListener() ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should do nothing if an invalid breakpoint is provided', () => { + expect( () => viewport.removeWithinBreakpointListener( 'unknown', () => '' ) ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should do nothing if no listener is provided', () => { + expect( () => viewport.removeWithinBreakpointListener( '<960px' ) ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should do nothing if an unregistered listener is provided', () => { + expect( () => viewport.removeWithinBreakpointListener( '<960px', () => '' ) ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should remove an added listener', () => { + const listener = jest.fn(); + viewport.addWithinBreakpointListener( '<960px', listener ); + // Get listener that actually got added. + const addedListener = + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + + expect( () => viewport.removeWithinBreakpointListener( '<960px', listener ) ).not.toThrow(); + expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 960px)', addedListener ); + } ); + } ); + + describe( 'removeIsMobileListener', () => { + test( 'should do nothing if nothing is provided', () => { + expect( () => viewport.removeIsMobileListener() ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should do nothing if an unregistered listener is provided', () => { + expect( () => viewport.removeIsMobileListener( () => '' ) ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should remove an added listener', () => { + const listener = jest.fn(); + viewport.addIsMobileListener( listener ); + // Get listener that actually got added. + const addedListener = + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + + expect( () => viewport.removeIsMobileListener( listener ) ).not.toThrow(); + expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', addedListener ); + } ); + } ); + + describe( 'removeIsDesktopListener', () => { + test( 'should do nothing if nothing is provided', () => { + expect( () => viewport.removeIsDesktopListener() ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should do nothing if an unregistered listener is provided', () => { + expect( () => viewport.removeIsDesktopListener( () => '' ) ).not.toThrow(); + expect( removeListenerMock ).not.toHaveBeenCalled(); + } ); + test( 'should remove an added listener', () => { + const listener = jest.fn(); + viewport.addIsDesktopListener( listener ); + // Get listener that actually got added. + const addedListener = + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + + expect( () => viewport.removeIsDesktopListener( listener ) ).not.toThrow(); + expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', addedListener ); + } ); + } ); +} ); From 369b741bee94bd3ee5e5275fa7cefd2d3f645db4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 12:25:14 +0000 Subject: [PATCH 03/23] Add helper hooks and higher order components to lib/viewport. Also refactors some components to make use of the new helpers. --- client/components/tooltip/index.jsx | 97 +++++++-------- ...-selected-site-premium-or-business-plan.js | 2 - client/lib/viewport/README.md | 54 ++++++-- client/lib/viewport/react-helpers.js | 116 ++++++++++++++++++ .../activity/activity-log-item/aggregated.jsx | 23 +--- .../activity/activity-log-item/index.jsx | 27 ++-- client/my-sites/plan-features/item.jsx | 6 +- 7 files changed, 232 insertions(+), 93 deletions(-) create mode 100644 client/lib/viewport/react-helpers.js diff --git a/client/components/tooltip/index.jsx b/client/components/tooltip/index.jsx index a7ce542a90c137..01c0650a1cc9ef 100644 --- a/client/components/tooltip/index.jsx +++ b/client/components/tooltip/index.jsx @@ -4,7 +4,7 @@ * External dependencies */ -import React, { Component } from 'react'; +import React from 'react'; import PropTypes from 'prop-types'; import classnames from 'classnames'; @@ -12,61 +12,58 @@ import classnames from 'classnames'; * Internal dependencies */ import Popover from 'components/popover'; -import { isMobile } from 'lib/viewport'; +import { useMobileBreakpoint } from 'lib/viewport/react-helpers'; -/** - * Module variables - */ const noop = () => {}; -class Tooltip extends Component { - static propTypes = { - autoPosition: PropTypes.bool, - className: PropTypes.string, - id: PropTypes.string, - isVisible: PropTypes.bool, - position: PropTypes.string, - rootClassName: PropTypes.string, - status: PropTypes.string, - showDelay: PropTypes.number, - showOnMobile: PropTypes.bool, - }; +function Tooltip( props ) { + const isMobile = useMobileBreakpoint(); - static defaultProps = { - showDelay: 100, - position: 'top', - showOnMobile: false, - }; - - render() { - if ( ! this.props.showOnMobile && isMobile() ) { - return null; - } + if ( ! props.showOnMobile && isMobile ) { + return null; + } - const classes = classnames( - 'popover', - 'tooltip', - `is-${ this.props.status }`, - `is-${ this.props.position }`, - this.props.className - ); + const classes = classnames( + 'popover', + 'tooltip', + `is-${ props.status }`, + `is-${ props.position }`, + props.className + ); - return ( - - { this.props.children } - - ); - } + return ( + + { props.children } + + ); } +Tooltip.propTypes = { + autoPosition: PropTypes.bool, + className: PropTypes.string, + id: PropTypes.string, + isVisible: PropTypes.bool, + position: PropTypes.string, + rootClassName: PropTypes.string, + status: PropTypes.string, + showDelay: PropTypes.number, + showOnMobile: PropTypes.bool, +}; + +Tooltip.defaultProps = { + showDelay: 100, + position: 'top', + showOnMobile: false, +}; + export default Tooltip; diff --git a/client/layout/guided-tours/docs/examples/selectors/has-selected-site-premium-or-business-plan.js b/client/layout/guided-tours/docs/examples/selectors/has-selected-site-premium-or-business-plan.js index bbd442621b1185..da97b8008609c7 100644 --- a/client/layout/guided-tours/docs/examples/selectors/has-selected-site-premium-or-business-plan.js +++ b/client/layout/guided-tours/docs/examples/selectors/has-selected-site-premium-or-business-plan.js @@ -12,8 +12,6 @@ import { PLAN_PREMIUM, PLAN_BUSINESS } from 'lib/plans/constants'; import { getSitePlan } from 'state/sites/selectors'; import { getSelectedSiteId } from 'state/ui/selectors'; -import { isDesktop } from 'lib/viewport'; - // NOTE: selector moved here because tour is no longer active and serves as example only // to use in a tour, move back to 'state/ui/guided-tours/contexts' (see commented out import above) /** diff --git a/client/lib/viewport/README.md b/client/lib/viewport/README.md index 84a0e84861f606..5c9a11e337424a 100644 --- a/client/lib/viewport/README.md +++ b/client/lib/viewport/README.md @@ -8,7 +8,7 @@ This module contains functions to identify the current viewport. This can be use Simple usage: ```js -import { isDesktop, isMobile } from 'viewport'; +import { isDesktop, isMobile } from 'lib/viewport'; if ( isDesktop() ) { // Render a component optimized for desktop view @@ -20,7 +20,7 @@ if ( isDesktop() ) { Using one of the other breakpoints: ```js -import { isWithinBreakpoint } from 'viewport'; +import { isWithinBreakpoint } from 'lib/viewport'; if ( isWithinBreakpoint( '>1400px' ) ) { // Render a component optimized for a very large screen @@ -32,7 +32,7 @@ if ( isWithinBreakpoint( '>1400px' ) ) { Registering to listen to changes: ```js -import { addIsDesktopListener, removeIsDesktopListener } from 'viewport'; +import { addIsDesktopListener, removeIsDesktopListener } from 'lib/viewport'; class MyComponent extends React.Component { sizeChanged = matches => { @@ -51,6 +51,34 @@ class MyComponent extends React.Component { } ``` +It also comes with React helpers that help in registering a component to breakpoint changes. + +Using a hook: + +```js +import { useMobileBreakpoint } from 'lib/viewport/react-helpers'; + +export default function MyComponent( props ) { + const isMobile = useMobileBreakpoint(); + return
Screen size: { isMobile ? 'mobile' : 'not mobile' }
; +} +``` + +Using a higher-order component: + +```js +import { withMobileBreakpoint } from 'lib/viewport/react-helpers'; + +class MyComponent extends React.Component { + render() { + const { isBreakpointActive: isMobile } = this.props; + return
Screen size: { isMobile ? 'mobile' : 'not mobile' }
; + } +} + +export default withMobileBreakpoint( MyComponent ); +``` + ### Supported methods - `isWithinBreakpoint( breakpoint )`: Whether the current screen size matches the breakpoint @@ -58,12 +86,24 @@ class MyComponent extends React.Component { - `isDesktop()`: Whether the current screen size matches a desktop breakpoint (>960px) - `addWithinBreakpointListener( breakpoint, listener )`: Register a listener for size changes that affect the breakpoint - `removeWithinBreakpointListener( breakpoint, listener )`: Unregister a previously registered listener -- `addIsMobileListener( breakpoint, listener )`: Register a listener for size changes that affect the mobile breakpoint (<480px) -- `removeIsMobileListener( breakpoint, listener )`: Unregister a previously registered listener -- `addIsDesktopListener( breakpoint, listener )`: Register a listener for size changes that affect the desktop breakpoint (>960px) -- `removeIsDesktopListener( breakpoint, listener )`: Unregister a previously registered listener +- `addIsMobileListener( listener )`: Register a listener for size changes that affect the mobile breakpoint (<480px) +- `removeIsMobileListener( listener )`: Unregister a previously registered listener for the mobile breakpoint (<480px) +- `addIsDesktopListener( listener )`: Register a listener for size changes that affect the desktop breakpoint (>960px) +- `removeIsDesktopListener( listener )`: Unregister a previously registered listener for the desktop breakpoint (>960px) - `getWindowInnerWidth()`: Get the inner width for the browser window. **Warning**: This method triggers a layout recalc, potentially resulting in performance issues. Please use a breakpoint instead wherever possible. +### Supported hooks + +- `useBreakpoint( breakpoint )`: Returns the current status for a breakpoint, and keeps it updated. +- `useMobileBreakpoint()`: Returns the current status for the mobile breakpoint, and keeps it updated. +- `useDesktopBreakpoint()`: Returns the current status for the desktop breakpoint, and keeps it updated. + +### Supported higher-order components + +- `withBreakpoint( WrappedComponent, breakpoint )`: Returns a wrapped component with the current status for a breakpoint as the `isBreakpointActive` prop. +- `useMobileBreakpoint( WrappedComponent )`: Returns a wrapped component with the current status for the mobile breakpoint as the `isBreakpointActive` prop. +- `useDesktopBreakpoint( WrappedComponent )`: Returns a wrapped component with the current status for the desktop breakpoint as the `isBreakpointActive` prop. + ### Supported breakpoints - '<480px' diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js new file mode 100644 index 00000000000000..1f4d03bdc6cf5f --- /dev/null +++ b/client/lib/viewport/react-helpers.js @@ -0,0 +1,116 @@ +/** @format */ +/** + * External dependencies + */ +import React, { useState, useEffect } from 'react'; + +/** + * Internal dependencies + */ +import { + isWithinBreakpoint, + addWithinBreakpointListener, + removeWithinBreakpointListener, + MOBILE_BREAKPOINT, + DESKTOP_BREAKPOINT, +} from './index'; + +/** + * React hook for getting the status for a breakpoint and keeping it updated. + * + * @param {String} breakpoint The breakpoint to consider. + * + * @returns {Boolean} The current status for the breakpoint. + */ +export function useBreakpoint( breakpoint ) { + const [ isActive, setIsActive ] = useState( isWithinBreakpoint( breakpoint ) ); + + function handleBreakpointChange( currentStatus ) { + setIsActive( currentStatus ); + } + + useEffect(() => { + addWithinBreakpointListener( breakpoint, handleBreakpointChange ); + + return function cleanup() { + removeWithinBreakpointListener( breakpoint, handleBreakpointChange ); + }; + }, []); + + return isActive; +} + +/** + * React hook for getting the status for the mobile breakpoint and keeping it + * updated. + * + * @returns {Boolean} The current status for the breakpoint. + */ +export function useMobileBreakpoint() { + return useBreakpoint( MOBILE_BREAKPOINT ); +} + +/** + * React hook for getting the status for the desktop breakpoint and keeping it + * updated. + * + * @returns {Boolean} The current status for the breakpoint. + */ +export function useDesktopBreakpoint() { + return useBreakpoint( DESKTOP_BREAKPOINT ); +} + +/** + * React higher order component for getting the status for a breakpoint and + * keeping it updated. + * + * @param {React.Component|Function} Wrapped The component to wrap. + * @param {String} breakpoint The breakpoint to consider. + * + * @returns {React.Component} The wrapped component. + */ +export function withBreakpoint( Wrapped, breakpoint ) { + return class extends React.Component { + state = { isActive: isWithinBreakpoint( breakpoint ) }; + + handleBreakpointChange = currentStatus => { + this.setState( { isActive: currentStatus } ); + }; + + componentDidMount() { + addWithinBreakpointListener( breakpoint, this.handleBreakpointChange ); + } + + componentWillUnmount() { + removeWithinBreakpointListener( breakpoint, this.handleBreakpointChange ); + } + + render() { + return ; + } + }; +} + +/** + * React higher order component for getting the status for the mobile + * breakpoint and keeping it updated. + * + * @param {React.Component|Function} Wrapped The component to wrap. + * + * @returns {React.Component} The wrapped component. + */ +export function withMobileBreakpoint( Wrapped ) { + return withBreakpoint( Wrapped, MOBILE_BREAKPOINT ); +} + +/** + * React higher order component for getting the status for the desktop + * breakpoint and keeping it updated. + * + * @param {React.Component|Function} Wrapped The component to wrap. + * + * @returns {React.Component} The wrapped component. + */ +export function withDesktopBreakpoint( Wrapped ) { + return withBreakpoint( Wrapped, DESKTOP_BREAKPOINT ); +} diff --git a/client/my-sites/activity/activity-log-item/aggregated.jsx b/client/my-sites/activity/activity-log-item/aggregated.jsx index 7648f9544b9f28..f1d64ae10de979 100644 --- a/client/my-sites/activity/activity-log-item/aggregated.jsx +++ b/client/my-sites/activity/activity-log-item/aggregated.jsx @@ -25,7 +25,7 @@ import { addQueryArgs } from 'lib/url'; import ActivityActor from './activity-actor'; import ActivityMedia from './activity-media'; import analytics from 'lib/analytics'; -import { isDesktop, addIsDesktopListener, removeIsDesktopListener } from 'lib/viewport'; +import { withDesktopBreakpoint } from 'lib/viewport/react-helpers'; const MAX_STREAM_ITEMS_IN_AGGREGATE = 10; @@ -71,20 +71,8 @@ class ActivityLogAggregatedItem extends Component { this.trackClick( 'view_all' ); }; - sizeChanged = () => { - this.forceUpdate(); - }; - - componentDidMount() { - addIsDesktopListener( this.sizeChanged ); - } - - componentWillUnmount() { - removeIsDesktopListener( this.sizeChanged ); - } - renderHeader() { - const { activity } = this.props; + const { activity, isBreakpointActive: isDesktop } = this.props; const { actorAvatarUrl, actorName, @@ -93,7 +81,6 @@ class ActivityLogAggregatedItem extends Component { multipleActors, activityMedia, } = activity; - const isDesktopSize = isDesktop(); let actor; if ( multipleActors ) { actor = ; @@ -104,7 +91,7 @@ class ActivityLogAggregatedItem extends Component { return (
{ actor } - { activityMedia && isDesktopSize && ( + { activityMedia && isDesktop && (
- { activityMedia && ! isDesktopSize && ( + { activityMedia && ! isDesktop && ( { export default connect( mapStateToProps, null -)( localize( ActivityLogAggregatedItem ) ); +)( withDesktopBreakpoint( localize( ActivityLogAggregatedItem ) ) ); diff --git a/client/my-sites/activity/activity-log-item/index.jsx b/client/my-sites/activity/activity-log-item/index.jsx index 5593a19451a2fc..9c288d527710d3 100644 --- a/client/my-sites/activity/activity-log-item/index.jsx +++ b/client/my-sites/activity/activity-log-item/index.jsx @@ -40,7 +40,7 @@ import getSiteGmtOffset from 'state/selectors/get-site-gmt-offset'; import getSiteTimezoneValue from 'state/selectors/get-site-timezone-value'; import { adjustMoment } from '../activity-log/utils'; import { getSite } from 'state/sites/selectors'; -import { isDesktop, addIsDesktopListener, removeIsDesktopListener } from 'lib/viewport'; +import { withDesktopBreakpoint } from 'lib/viewport/react-helpers'; class ActivityLogItem extends Component { static propTypes = { @@ -96,23 +96,22 @@ class ActivityLogItem extends Component { this.forceUpdate(); }; - componentDidMount() { - addIsDesktopListener( this.sizeChanged ); - } - - componentWillUnmount() { - removeIsDesktopListener( this.sizeChanged ); - } - renderHeader() { const { - activity: { activityTitle, actorAvatarUrl, actorName, actorRole, actorType, activityMedia }, + activity: { + activityTitle, + actorAvatarUrl, + actorName, + actorRole, + actorType, + activityMedia, + isBreakpointActive: isDesktop, + }, } = this.props; - const isDesktopSize = isDesktop(); return (
- { activityMedia && isDesktopSize && ( + { activityMedia && isDesktop && (
{ activityTitle }
- { activityMedia && ! isDesktopSize && ( + { activityMedia && ! isDesktop && ( export default connect( mapStateToProps, mapDispatchToProps -)( localize( ActivityLogItem ) ); +)( withDesktopBreakpoint( localize( ActivityLogItem ) ) ); diff --git a/client/my-sites/plan-features/item.jsx b/client/my-sites/plan-features/item.jsx index d90527c0d90680..ed840a177b7a87 100644 --- a/client/my-sites/plan-features/item.jsx +++ b/client/my-sites/plan-features/item.jsx @@ -11,9 +11,11 @@ import Gridicon from 'gridicons'; * Internal dependencies */ import InfoPopover from 'components/info-popover'; -import { isMobile } from 'lib/viewport'; +import { useMobileBreakpoint } from 'lib/viewport/react-helpers'; export default function PlanFeaturesItem( { children, description, hideInfoPopover } ) { + const isMobile = useMobileBreakpoint(); + return (
@@ -21,7 +23,7 @@ export default function PlanFeaturesItem( { children, description, hideInfoPopov { hideInfoPopover ? null : ( { description } From cd000ec526a9b97c14624d6822d9717166f55b36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 14:40:53 +0000 Subject: [PATCH 04/23] Add tests for lib/viewport hooks and HOCs. --- client/lib/viewport/test/index.js | 4 + client/lib/viewport/test/react-helpers.js | 209 ++++++++++++++++++++++ 2 files changed, 213 insertions(+) create mode 100644 client/lib/viewport/test/react-helpers.js diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js index 19c0cf8dc5598c..7a959ebaf787b6 100644 --- a/client/lib/viewport/test/index.js +++ b/client/lib/viewport/test/index.js @@ -24,6 +24,10 @@ const matchMediaMock = jest.fn( query => { return mediaListObjectMock; } ); +// Disable console warnings for this file. +// eslint-disable-next-line no-console +console.warn = jest.fn(); + describe( 'viewport', () => { beforeAll( async () => { window.matchMedia = matchMediaMock; diff --git a/client/lib/viewport/test/react-helpers.js b/client/lib/viewport/test/react-helpers.js new file mode 100644 index 00000000000000..a0d31dae290ce4 --- /dev/null +++ b/client/lib/viewport/test/react-helpers.js @@ -0,0 +1,209 @@ +/** + * @format + * @jest-environment jsdom + */ + +/** + * Internal dependencies + */ +import React from 'react'; +import ReactDOM from 'react-dom'; +import { act } from 'react-dom/test-utils'; + +let helpers; + +const listeners = {}; + +const matchesMock = jest.fn( () => true ); +const addListenerMock = jest.fn( ( query, listener ) => { + if ( listeners[ query ] ) { + listeners[ query ].push( listener ); + } else { + listeners[ query ] = [ listener ]; + } +} ); +const removeListenerMock = jest.fn( ( query, listener ) => { + if ( listeners[ query ] ) { + listeners[ query ] = listeners[ query ].filter( item => item !== listener ); + } +} ); + +function callQueryListeners( query, value ) { + for ( const listener of listeners[ query ] ) { + listener( { matches: value } ); + } +} + +const matchMediaMock = jest.fn( query => { + const mediaListObjectMock = { + addListener: listener => addListenerMock( query, listener ), + removeListener: listener => removeListenerMock( query, listener ), + }; + // Add matches read-only property. + Object.defineProperty( mediaListObjectMock, 'matches', { + get: () => matchesMock( query ), + } ); + return mediaListObjectMock; +} ); + +// Disable console warnings for this file. +// eslint-disable-next-line no-console +console.warn = jest.fn(); + +describe( 'viewport/react-helpers', () => { + let container; + + // Auxiliary method to test a valid component. + function runComponentTests( TestComponent, query ) { + // Test initial state (defaults to true). + act( () => { + ReactDOM.render( , container ); + } ); + + expect( container.textContent ).toBe( 'true' ); + expect( listeners[ query ] ).not.toBe( undefined ); + expect( listeners[ query ].length ).toBe( 1 ); + + // Simulate a window resize by calling the registered listeners for a query + // with a different value (false). + act( () => { + callQueryListeners( query, false ); + } ); + + expect( container.textContent ).toBe( 'false' ); + + // Ensure that listeners are cleaned up when the component unmounts. + act( () => { + ReactDOM.render(
, container ); + } ); + + expect( listeners[ query ].length ).toBe( 0 ); + } + + // Auxiliary class for HOC tests. + class BaseComponent extends React.Component { + render() { + const isActive = this.props.isBreakpointActive; + return isActive ? 'true' : 'false'; + } + } + + beforeAll( async () => { + window.matchMedia = matchMediaMock; + helpers = await import( '../react-helpers' ); + } ); + + beforeEach( () => { + container = document.createElement( 'div' ); + matchesMock.mockClear(); + addListenerMock.mockClear(); + removeListenerMock.mockClear(); + } ); + + describe( 'useBreakpoint', () => { + test( 'returns undefined when called with no breakpoint', () => { + function TestComponent() { + const isActive = helpers.useBreakpoint(); + return isActive === undefined ? 'undefined' : `unexpected value: ${ isActive }`; + } + + act( () => { + ReactDOM.render( , container ); + } ); + + expect( container.textContent ).toBe( 'undefined' ); + } ); + + test( 'returns undefined for an unknown breakpoint', () => { + function TestComponent() { + const isActive = helpers.useBreakpoint( 'unknown' ); + return isActive === undefined ? 'undefined' : `unexpected value: ${ isActive }`; + } + + act( () => { + ReactDOM.render( , container ); + } ); + + expect( container.textContent ).toBe( 'undefined' ); + } ); + + test( 'returns the current breakpoint state for a valid breakpoint', () => { + function TestComponent() { + const isActive = helpers.useBreakpoint( '<960px' ); + return isActive ? 'true' : 'false'; + } + + runComponentTests( TestComponent, '(max-width: 960px)' ); + } ); + } ); + + describe( 'useMobileBreakpoint', () => { + test( 'returns the current breakpoint state for the mobile breakpoint', () => { + function TestComponent() { + const isActive = helpers.useMobileBreakpoint(); + return isActive ? 'true' : 'false'; + } + + runComponentTests( TestComponent, '(max-width: 480px)' ); + } ); + } ); + + describe( 'useDesktopBreakpoint', () => { + test( 'returns the current breakpoint state for the desktop breakpoint', () => { + function TestComponent() { + const isActive = helpers.useDesktopBreakpoint(); + return isActive ? 'true' : 'false'; + } + + runComponentTests( TestComponent, '(min-width: 961px)' ); + } ); + } ); + + describe( 'withBreakpoint', () => { + class ExpectUndefinedComponent extends React.Component { + render() { + const isActive = this.props.isBreakpointActive; + return isActive === undefined ? 'undefined' : `unexpected value: ${ isActive }`; + } + } + + test( 'returns undefined when called with no breakpoint', () => { + const TestComponent = helpers.withBreakpoint( ExpectUndefinedComponent ); + + act( () => { + ReactDOM.render( , container ); + } ); + + expect( container.textContent ).toBe( 'undefined' ); + } ); + + test( 'returns undefined for an unknown breakpoint', () => { + const TestComponent = helpers.withBreakpoint( ExpectUndefinedComponent, 'unknown' ); + + act( () => { + ReactDOM.render( , container ); + } ); + + expect( container.textContent ).toBe( 'undefined' ); + } ); + + test( 'returns the current breakpoint state for a valid breakpoint', () => { + const TestComponent = helpers.withBreakpoint( BaseComponent, '<960px' ); + runComponentTests( TestComponent, '(max-width: 960px)' ); + } ); + } ); + + describe( 'withMobileBreakpoint', () => { + test( 'returns the current breakpoint state for the mobile breakpoint', () => { + const TestComponent = helpers.withMobileBreakpoint( BaseComponent ); + runComponentTests( TestComponent, '(max-width: 480px)' ); + } ); + } ); + + describe( 'withDesktopBreakpoint', () => { + test( 'returns the current breakpoint state for the desktop breakpoint', () => { + const TestComponent = helpers.withDesktopBreakpoint( BaseComponent ); + runComponentTests( TestComponent, '(min-width: 961px)' ); + } ); + } ); +} ); From 80cf057d6ce1bcfe6cbfb400178fb29c9aa0db0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 15:04:25 +0000 Subject: [PATCH 05/23] Safely disable and restore console.warn for lib/viewport tests. --- client/lib/viewport/test/index.js | 10 ++++++---- client/lib/viewport/test/react-helpers.js | 10 ++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js index 7a959ebaf787b6..4cba76e7c7daab 100644 --- a/client/lib/viewport/test/index.js +++ b/client/lib/viewport/test/index.js @@ -24,14 +24,12 @@ const matchMediaMock = jest.fn( query => { return mediaListObjectMock; } ); -// Disable console warnings for this file. -// eslint-disable-next-line no-console -console.warn = jest.fn(); - describe( 'viewport', () => { beforeAll( async () => { window.matchMedia = matchMediaMock; viewport = await import( '..' ); + // Disable console warnings. + jest.spyOn( console, 'warn' ).mockImplementation( () => '' ); } ); beforeEach( () => { @@ -228,4 +226,8 @@ describe( 'viewport', () => { expect( removeListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', addedListener ); } ); } ); + + afterAll( () => { + jest.restoreAllMocks(); + } ); } ); diff --git a/client/lib/viewport/test/react-helpers.js b/client/lib/viewport/test/react-helpers.js index a0d31dae290ce4..69fd15fcfedfa2 100644 --- a/client/lib/viewport/test/react-helpers.js +++ b/client/lib/viewport/test/react-helpers.js @@ -46,10 +46,6 @@ const matchMediaMock = jest.fn( query => { return mediaListObjectMock; } ); -// Disable console warnings for this file. -// eslint-disable-next-line no-console -console.warn = jest.fn(); - describe( 'viewport/react-helpers', () => { let container; @@ -91,6 +87,8 @@ describe( 'viewport/react-helpers', () => { beforeAll( async () => { window.matchMedia = matchMediaMock; helpers = await import( '../react-helpers' ); + // Disable console warnings. + jest.spyOn( console, 'warn' ).mockImplementation( () => '' ); } ); beforeEach( () => { @@ -206,4 +204,8 @@ describe( 'viewport/react-helpers', () => { runComponentTests( TestComponent, '(min-width: 961px)' ); } ); } ); + + afterAll( () => { + jest.restoreAllMocks(); + } ); } ); From 3aba4e04e89e19a64c0a63a3dc5908bbc89f2c17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 15:16:41 +0000 Subject: [PATCH 06/23] Add missing JSDoc to lib/viewport. --- client/lib/viewport/index.js | 55 +++++++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/client/lib/viewport/index.js b/client/lib/viewport/index.js index 671bce41ed07ac..74a651426976c9 100644 --- a/client/lib/viewport/index.js +++ b/client/lib/viewport/index.js @@ -102,11 +102,22 @@ function getMediaQueryList( breakpoint ) { return mediaQueryLists[ breakpoint ]; } +/** + * Returns whether the current window width matches a breakpoint. + * @param {String} breakpoint The breakpoint to consider. + * + * @returns {Boolean} Whether the provided breakpoint is matched. + */ export function isWithinBreakpoint( breakpoint ) { const mediaQueryList = getMediaQueryList( breakpoint ); return mediaQueryList ? mediaQueryList.matches : undefined; } +/** + * Registers a listener to be notified of changes to breakpoint matching status. + * @param {String} breakpoint The breakpoint to consider. + * @param {Function} listener The listener to be called on change. + */ export function addWithinBreakpointListener( breakpoint, listener ) { if ( ! listener ) { return; @@ -120,6 +131,11 @@ export function addWithinBreakpointListener( breakpoint, listener ) { } } +/** + * Removes a listener that was being notified of changes to breakpoint matching status. + * @param {String} breakpoint The breakpoint to consider. + * @param {Function} listener The listener to be called on change. + */ export function removeWithinBreakpointListener( breakpoint, listener ) { if ( ! listener ) { return; @@ -133,30 +149,61 @@ export function removeWithinBreakpointListener( breakpoint, listener ) { } } +/** + * Returns whether the current window width matches the mobile breakpoint. + * + * @returns {Boolean} Whether the mobile breakpoint is matched. + */ export function isMobile() { return isWithinBreakpoint( MOBILE_BREAKPOINT ); } +/** + * Registers a listener to be notified of changes to mobile breakpoint matching status. + * @param {Function} listener The listener to be called on change. + */ export function addIsMobileListener( listener ) { - return addWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); + addWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); } +/** + * Removes a listener that was being notified of changes to mobile breakpoint matching status. + * @param {Function} listener The listener to be called on change. + */ export function removeIsMobileListener( listener ) { - return removeWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); + removeWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); } +/** + * Returns whether the current window width matches the desktop breakpoint. + * + * @returns {Boolean} Whether the desktop breakpoint is matched. + */ export function isDesktop() { return isWithinBreakpoint( DESKTOP_BREAKPOINT ); } +/** + * Registers a listener to be notified of changes to desktop breakpoint matching status. + * @param {Function} listener The listener to be called on change. + */ export function addIsDesktopListener( listener ) { - return addWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); + addWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); } +/** + * Removes a listener that was being notified of changes to desktop breakpoint matching status. + * @param {Function} listener The listener to be called on change. + */ export function removeIsDesktopListener( listener ) { - return removeWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); + removeWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); } +/** + * Returns the current window width. + * Avoid using this method, as it triggers a layout recalc. + * @returns {Number} The current window width, in pixels. + */ export function getWindowInnerWidth() { return isServer ? SERVER_WIDTH : window.innerWidth; } From 4a89ba9d63399e70bd2a08bf4c406c3debe89498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 15:46:39 +0000 Subject: [PATCH 07/23] Improve displayNames for lib/viewport HOCs --- client/lib/viewport/react-helpers.js | 33 ++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index 1f4d03bdc6cf5f..0c04accb4534cb 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -3,6 +3,7 @@ * External dependencies */ import React, { useState, useEffect } from 'react'; +import { camelCase, upperFirst } from 'lodash'; /** * Internal dependencies @@ -61,16 +62,16 @@ export function useDesktopBreakpoint() { } /** - * React higher order component for getting the status for a breakpoint and - * keeping it updated. - * + * Auxiliary method to produce a higher order component for getting the status + * for a breakpoint and keeping it updated. It also sets the component name. * @param {React.Component|Function} Wrapped The component to wrap. * @param {String} breakpoint The breakpoint to consider. + * @param {String} modifierName The name to modify the component with. * * @returns {React.Component} The wrapped component. */ -export function withBreakpoint( Wrapped, breakpoint ) { - return class extends React.Component { +function withBreakpointAux( Wrapped, breakpoint, modifierName ) { + const EnhancedComponent = class extends React.Component { state = { isActive: isWithinBreakpoint( breakpoint ) }; handleBreakpointChange = currentStatus => { @@ -89,6 +90,24 @@ export function withBreakpoint( Wrapped, breakpoint ) { return ; } }; + + const { displayName = Wrapped.name || 'Component' } = Wrapped; + EnhancedComponent.displayName = `${ upperFirst( camelCase( modifierName ) ) }(${ displayName })`; + + return EnhancedComponent; +} + +/** + * React higher order component for getting the status for a breakpoint and + * keeping it updated. + * + * @param {React.Component|Function} Wrapped The component to wrap. + * @param {String} breakpoint The breakpoint to consider. + * + * @returns {React.Component} The wrapped component. + */ +export function withBreakpoint( Wrapped, breakpoint ) { + return withBreakpointAux( Wrapped, breakpoint, 'WithBreakpoint' ); } /** @@ -100,7 +119,7 @@ export function withBreakpoint( Wrapped, breakpoint ) { * @returns {React.Component} The wrapped component. */ export function withMobileBreakpoint( Wrapped ) { - return withBreakpoint( Wrapped, MOBILE_BREAKPOINT ); + return withBreakpointAux( Wrapped, MOBILE_BREAKPOINT, 'WithMobileBreakpoint' ); } /** @@ -112,5 +131,5 @@ export function withMobileBreakpoint( Wrapped ) { * @returns {React.Component} The wrapped component. */ export function withDesktopBreakpoint( Wrapped ) { - return withBreakpoint( Wrapped, DESKTOP_BREAKPOINT ); + return withBreakpointAux( Wrapped, DESKTOP_BREAKPOINT, 'WithDesktopBreakpoint' ); } From f5b063da4329a5e9a57e7e75152a8e2c7dddaa8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 17:10:50 +0000 Subject: [PATCH 08/23] Change lib/viewport listener API to use subscription tokens. This allows the same listener to be used more than once. Also reimplements `withBreakpoint` using `useBreakpoint`. --- client/lib/viewport/README.md | 10 ++++---- client/lib/viewport/index.js | 32 +++++++++++++---------- client/lib/viewport/react-helpers.js | 25 ++++-------------- client/lib/viewport/test/index.js | 38 ++++++++++------------------ 4 files changed, 43 insertions(+), 62 deletions(-) diff --git a/client/lib/viewport/README.md b/client/lib/viewport/README.md index 5c9a11e337424a..cc26071a50320d 100644 --- a/client/lib/viewport/README.md +++ b/client/lib/viewport/README.md @@ -42,11 +42,11 @@ class MyComponent extends React.Component { }; componentDidMount() { - addIsDesktopListener( this.sizeChanged ); + this.subscription = addIsDesktopListener( this.sizeChanged ); } componentWillUnmount() { - removeIsDesktopListener( this.sizeChanged ); + removeIsDesktopListener( this.subscription ); } } ``` @@ -85,11 +85,11 @@ export default withMobileBreakpoint( MyComponent ); - `isMobile()`: Whether the current screen size matches a mobile breakpoint (<480px) - `isDesktop()`: Whether the current screen size matches a desktop breakpoint (>960px) - `addWithinBreakpointListener( breakpoint, listener )`: Register a listener for size changes that affect the breakpoint -- `removeWithinBreakpointListener( breakpoint, listener )`: Unregister a previously registered listener +- `removeWithinBreakpointListener( breakpoint, subscription )`: Unregister a previously registered subscription - `addIsMobileListener( listener )`: Register a listener for size changes that affect the mobile breakpoint (<480px) -- `removeIsMobileListener( listener )`: Unregister a previously registered listener for the mobile breakpoint (<480px) +- `removeIsMobileListener( subscription )`: Unregister a previously registered subscription for the mobile breakpoint (<480px) - `addIsDesktopListener( listener )`: Register a listener for size changes that affect the desktop breakpoint (>960px) -- `removeIsDesktopListener( listener )`: Unregister a previously registered listener for the desktop breakpoint (>960px) +- `removeIsDesktopListener( subscription )`: Unregister a previously registered subscription for the desktop breakpoint (>960px) - `getWindowInnerWidth()`: Get the inner width for the browser window. **Warning**: This method triggers a layout recalc, potentially resulting in performance issues. Please use a breakpoint instead wherever possible. ### Supported hooks diff --git a/client/lib/viewport/index.js b/client/lib/viewport/index.js index 74a651426976c9..eaa3d1cda36d0c 100644 --- a/client/lib/viewport/index.js +++ b/client/lib/viewport/index.js @@ -88,8 +88,6 @@ const mediaQueryLists = { '480px-960px': createMediaQueryList( { min: 480, max: 960 } ), }; -const listeners = {}; - function getMediaQueryList( breakpoint ) { if ( ! mediaQueryLists.hasOwnProperty( breakpoint ) ) { try { @@ -117,6 +115,8 @@ export function isWithinBreakpoint( breakpoint ) { * Registers a listener to be notified of changes to breakpoint matching status. * @param {String} breakpoint The breakpoint to consider. * @param {Function} listener The listener to be called on change. + * + * @returns {Function} The registered subscription; undefined if none. */ export function addWithinBreakpointListener( breakpoint, listener ) { if ( ! listener ) { @@ -125,27 +125,29 @@ export function addWithinBreakpointListener( breakpoint, listener ) { const mediaQueryList = getMediaQueryList( breakpoint ); - if ( mediaQueryList && ! isServer && ! listeners[ listener ] ) { - listeners[ listener ] = evt => listener( evt.matches ); - mediaQueryList.addListener( listeners[ listener ] ); + if ( mediaQueryList && ! isServer ) { + const wrappedListener = evt => listener( evt.matches ); + mediaQueryList.addListener( wrappedListener ); + return wrappedListener; } + + return undefined; } /** * Removes a listener that was being notified of changes to breakpoint matching status. * @param {String} breakpoint The breakpoint to consider. - * @param {Function} listener The listener to be called on change. + * @param {Function} subscription The subscription to be removed. */ -export function removeWithinBreakpointListener( breakpoint, listener ) { - if ( ! listener ) { +export function removeWithinBreakpointListener( breakpoint, subscription ) { + if ( ! subscription ) { return; } const mediaQueryList = getMediaQueryList( breakpoint ); - if ( mediaQueryList && ! isServer && listeners[ listener ] ) { - mediaQueryList.removeListener( listeners[ listener ] ); - delete listeners[ listener ]; + if ( mediaQueryList && ! isServer && subscription ) { + mediaQueryList.removeListener( subscription ); } } @@ -161,9 +163,11 @@ export function isMobile() { /** * Registers a listener to be notified of changes to mobile breakpoint matching status. * @param {Function} listener The listener to be called on change. + * + * @returns {Function} The registered subscription; undefined if none. */ export function addIsMobileListener( listener ) { - addWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); + return addWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); } /** @@ -186,9 +190,11 @@ export function isDesktop() { /** * Registers a listener to be notified of changes to desktop breakpoint matching status. * @param {Function} listener The listener to be called on change. + * + * @returns {Function} The registered subscription; undefined if none. */ export function addIsDesktopListener( listener ) { - addWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); + return addWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); } /** diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index 0c04accb4534cb..0ac334d5d0339d 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -31,10 +31,10 @@ export function useBreakpoint( breakpoint ) { } useEffect(() => { - addWithinBreakpointListener( breakpoint, handleBreakpointChange ); + const subscription = addWithinBreakpointListener( breakpoint, handleBreakpointChange ); return function cleanup() { - removeWithinBreakpointListener( breakpoint, handleBreakpointChange ); + removeWithinBreakpointListener( breakpoint, subscription ); }; }, []); @@ -71,24 +71,9 @@ export function useDesktopBreakpoint() { * @returns {React.Component} The wrapped component. */ function withBreakpointAux( Wrapped, breakpoint, modifierName ) { - const EnhancedComponent = class extends React.Component { - state = { isActive: isWithinBreakpoint( breakpoint ) }; - - handleBreakpointChange = currentStatus => { - this.setState( { isActive: currentStatus } ); - }; - - componentDidMount() { - addWithinBreakpointListener( breakpoint, this.handleBreakpointChange ); - } - - componentWillUnmount() { - removeWithinBreakpointListener( breakpoint, this.handleBreakpointChange ); - } - - render() { - return ; - } + const EnhancedComponent = function( props ) { + const isActive = useBreakpoint( breakpoint ); + return ; }; const { displayName = Wrapped.name || 'Component' } = Wrapped; diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js index 4cba76e7c7daab..f00276ce345885 100644 --- a/client/lib/viewport/test/index.js +++ b/client/lib/viewport/test/index.js @@ -166,20 +166,18 @@ describe( 'viewport', () => { expect( () => viewport.removeWithinBreakpointListener( '<960px' ) ).not.toThrow(); expect( removeListenerMock ).not.toHaveBeenCalled(); } ); - test( 'should do nothing if an unregistered listener is provided', () => { + test( 'should not throw if an unregistered listener is provided', () => { expect( () => viewport.removeWithinBreakpointListener( '<960px', () => '' ) ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); } ); test( 'should remove an added listener', () => { const listener = jest.fn(); - viewport.addWithinBreakpointListener( '<960px', listener ); - // Get listener that actually got added. - const addedListener = - addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + const subscription = viewport.addWithinBreakpointListener( '<960px', listener ); - expect( () => viewport.removeWithinBreakpointListener( '<960px', listener ) ).not.toThrow(); + expect( () => + viewport.removeWithinBreakpointListener( '<960px', subscription ) + ).not.toThrow(); expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); - expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 960px)', addedListener ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 960px)', subscription ); } ); } ); @@ -188,20 +186,16 @@ describe( 'viewport', () => { expect( () => viewport.removeIsMobileListener() ).not.toThrow(); expect( removeListenerMock ).not.toHaveBeenCalled(); } ); - test( 'should do nothing if an unregistered listener is provided', () => { + test( 'should not throw if an unregistered listener is provided', () => { expect( () => viewport.removeIsMobileListener( () => '' ) ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); } ); test( 'should remove an added listener', () => { const listener = jest.fn(); - viewport.addIsMobileListener( listener ); - // Get listener that actually got added. - const addedListener = - addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + const subscription = viewport.addIsMobileListener( listener ); - expect( () => viewport.removeIsMobileListener( listener ) ).not.toThrow(); + expect( () => viewport.removeIsMobileListener( subscription ) ).not.toThrow(); expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); - expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', addedListener ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', subscription ); } ); } ); @@ -210,20 +204,16 @@ describe( 'viewport', () => { expect( () => viewport.removeIsDesktopListener() ).not.toThrow(); expect( removeListenerMock ).not.toHaveBeenCalled(); } ); - test( 'should do nothing if an unregistered listener is provided', () => { + test( 'should not throw if an unregistered listener is provided', () => { expect( () => viewport.removeIsDesktopListener( () => '' ) ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); } ); test( 'should remove an added listener', () => { const listener = jest.fn(); - viewport.addIsDesktopListener( listener ); - // Get listener that actually got added. - const addedListener = - addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + const subscription = viewport.addIsDesktopListener( listener ); - expect( () => viewport.removeIsDesktopListener( listener ) ).not.toThrow(); + expect( () => viewport.removeIsDesktopListener( subscription ) ).not.toThrow(); expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); - expect( removeListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', addedListener ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', subscription ); } ); } ); From a5f339ae934e705f50bd0aaa10a35a0c1e466059 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 17:28:39 +0000 Subject: [PATCH 09/23] Change lib/viewport helper tests to create multiple instances. --- client/lib/viewport/test/react-helpers.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/client/lib/viewport/test/react-helpers.js b/client/lib/viewport/test/react-helpers.js index 69fd15fcfedfa2..e48909a50ce172 100644 --- a/client/lib/viewport/test/react-helpers.js +++ b/client/lib/viewport/test/react-helpers.js @@ -53,12 +53,19 @@ describe( 'viewport/react-helpers', () => { function runComponentTests( TestComponent, query ) { // Test initial state (defaults to true). act( () => { - ReactDOM.render( , container ); + ReactDOM.render( +
+ + + +
, + container + ); } ); - expect( container.textContent ).toBe( 'true' ); + expect( container.textContent ).toBe( 'truetruetrue' ); expect( listeners[ query ] ).not.toBe( undefined ); - expect( listeners[ query ].length ).toBe( 1 ); + expect( listeners[ query ].length ).toBe( 3 ); // Simulate a window resize by calling the registered listeners for a query // with a different value (false). @@ -66,7 +73,7 @@ describe( 'viewport/react-helpers', () => { callQueryListeners( query, false ); } ); - expect( container.textContent ).toBe( 'false' ); + expect( container.textContent ).toBe( 'falsefalsefalse' ); // Ensure that listeners are cleaned up when the component unmounts. act( () => { From fc9e5a8126fe87800c5d4154c9771d947e712032 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Wed, 27 Feb 2019 18:06:13 +0000 Subject: [PATCH 10/23] Small cleanups to lib/viewport. --- client/lib/viewport/index.js | 2 +- client/lib/viewport/test/index.js | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/client/lib/viewport/index.js b/client/lib/viewport/index.js index eaa3d1cda36d0c..1f9207edeef38c 100644 --- a/client/lib/viewport/index.js +++ b/client/lib/viewport/index.js @@ -146,7 +146,7 @@ export function removeWithinBreakpointListener( breakpoint, subscription ) { const mediaQueryList = getMediaQueryList( breakpoint ); - if ( mediaQueryList && ! isServer && subscription ) { + if ( mediaQueryList && ! isServer ) { mediaQueryList.removeListener( subscription ); } } diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js index f00276ce345885..695a5a68132e32 100644 --- a/client/lib/viewport/test/index.js +++ b/client/lib/viewport/test/index.js @@ -86,7 +86,10 @@ describe( 'viewport', () => { test( 'should add a listener for a valid breakpoint', () => { const listener = jest.fn(); const event = { matches: 'bar' }; - expect( () => viewport.addWithinBreakpointListener( '<960px', listener ) ).not.toThrow(); + let subscription; + expect( + () => ( subscription = viewport.addWithinBreakpointListener( '<960px', listener ) ) + ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( '(max-width: 960px)', @@ -98,7 +101,7 @@ describe( 'viewport', () => { expect( listener ).toHaveBeenCalledWith( 'bar' ); // Clean up. try { - viewport.removeWithinBreakpointListener( '<960px', listener ); + viewport.removeWithinBreakpointListener( '<960px', subscription ); } catch {} } ); } ); @@ -111,7 +114,8 @@ describe( 'viewport', () => { test( 'should add a listener', () => { const listener = jest.fn(); const event = { matches: 'bar' }; - expect( () => viewport.addIsMobileListener( listener ) ).not.toThrow(); + let subscription; + expect( () => ( subscription = viewport.addIsMobileListener( listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', @@ -123,7 +127,7 @@ describe( 'viewport', () => { expect( listener ).toHaveBeenCalledWith( 'bar' ); // Clean up. try { - viewport.removeIsMobileListener( listener ); + viewport.removeIsMobileListener( subscription ); } catch {} } ); } ); @@ -136,7 +140,8 @@ describe( 'viewport', () => { test( 'should add a listener', () => { const listener = jest.fn(); const event = { matches: 'bar' }; - expect( () => viewport.addIsDesktopListener( listener ) ).not.toThrow(); + let subscription; + expect( () => ( subscription = viewport.addIsDesktopListener( listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', @@ -148,7 +153,7 @@ describe( 'viewport', () => { expect( listener ).toHaveBeenCalledWith( 'bar' ); // Clean up. try { - viewport.removeIsDesktopListener( listener ); + viewport.removeIsDesktopListener( subscription ); } catch {} } ); } ); From c195fbfbbc65677971f5092b6489ffaabab0ff1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Thu, 28 Feb 2019 11:24:24 +0000 Subject: [PATCH 11/23] Move afterAll closer to other methods in lib/viewport tests. --- client/lib/viewport/test/index.js | 8 ++++---- client/lib/viewport/test/react-helpers.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js index 695a5a68132e32..8577efae981f76 100644 --- a/client/lib/viewport/test/index.js +++ b/client/lib/viewport/test/index.js @@ -38,6 +38,10 @@ describe( 'viewport', () => { removeListenerMock.mockClear(); } ); + afterAll( () => { + jest.restoreAllMocks(); + } ); + describe( 'isWithinBreakpoint', () => { test( 'should return undefined when called with no breakpoint', () => { expect( viewport.isWithinBreakpoint() ).toBe( undefined ); @@ -221,8 +225,4 @@ describe( 'viewport', () => { expect( removeListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', subscription ); } ); } ); - - afterAll( () => { - jest.restoreAllMocks(); - } ); } ); diff --git a/client/lib/viewport/test/react-helpers.js b/client/lib/viewport/test/react-helpers.js index e48909a50ce172..8227d633664cc1 100644 --- a/client/lib/viewport/test/react-helpers.js +++ b/client/lib/viewport/test/react-helpers.js @@ -105,6 +105,10 @@ describe( 'viewport/react-helpers', () => { removeListenerMock.mockClear(); } ); + afterAll( () => { + jest.restoreAllMocks(); + } ); + describe( 'useBreakpoint', () => { test( 'returns undefined when called with no breakpoint', () => { function TestComponent() { @@ -211,8 +215,4 @@ describe( 'viewport/react-helpers', () => { runComponentTests( TestComponent, '(min-width: 961px)' ); } ); } ); - - afterAll( () => { - jest.restoreAllMocks(); - } ); } ); From 8cd491adeeb9e2a310b6b73b2a9372ed6beca059 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Fri, 1 Mar 2019 13:44:53 +0000 Subject: [PATCH 12/23] Change the lib/viewport registration API to return the unsub fn. This is much cleaner than maintaining a bunch of unsubscribe methods and storing a pointer too. --- client/lib/viewport/README.md | 23 +++--- client/lib/viewport/index.js | 38 +--------- client/lib/viewport/react-helpers.js | 7 +- client/lib/viewport/test/index.js | 104 ++++++--------------------- 4 files changed, 41 insertions(+), 131 deletions(-) diff --git a/client/lib/viewport/README.md b/client/lib/viewport/README.md index cc26071a50320d..820547e0c8b03f 100644 --- a/client/lib/viewport/README.md +++ b/client/lib/viewport/README.md @@ -32,7 +32,7 @@ if ( isWithinBreakpoint( '>1400px' ) ) { Registering to listen to changes: ```js -import { addIsDesktopListener, removeIsDesktopListener } from 'lib/viewport'; +import { addIsDesktopListener } from 'lib/viewport'; class MyComponent extends React.Component { sizeChanged = matches => { @@ -42,11 +42,13 @@ class MyComponent extends React.Component { }; componentDidMount() { - this.subscription = addIsDesktopListener( this.sizeChanged ); + this.unsubscribe = addIsDesktopListener( this.sizeChanged ); } componentWillUnmount() { - removeIsDesktopListener( this.subscription ); + if ( this.unsubscribe ) { + this.unsubscribe(); + } } } ``` @@ -81,15 +83,12 @@ export default withMobileBreakpoint( MyComponent ); ### Supported methods -- `isWithinBreakpoint( breakpoint )`: Whether the current screen size matches the breakpoint -- `isMobile()`: Whether the current screen size matches a mobile breakpoint (<480px) -- `isDesktop()`: Whether the current screen size matches a desktop breakpoint (>960px) -- `addWithinBreakpointListener( breakpoint, listener )`: Register a listener for size changes that affect the breakpoint -- `removeWithinBreakpointListener( breakpoint, subscription )`: Unregister a previously registered subscription -- `addIsMobileListener( listener )`: Register a listener for size changes that affect the mobile breakpoint (<480px) -- `removeIsMobileListener( subscription )`: Unregister a previously registered subscription for the mobile breakpoint (<480px) -- `addIsDesktopListener( listener )`: Register a listener for size changes that affect the desktop breakpoint (>960px) -- `removeIsDesktopListener( subscription )`: Unregister a previously registered subscription for the desktop breakpoint (>960px) +- `isWithinBreakpoint( breakpoint )`: Whether the current screen size matches the breakpoint. +- `isMobile()`: Whether the current screen size matches a mobile breakpoint (<480px). +- `isDesktop()`: Whether the current screen size matches a desktop breakpoint (>960px). +- `addWithinBreakpointListener( breakpoint, listener )`: Register a listener for size changes that affect the breakpoint. Returns the unsubscribe function. +- `addIsMobileListener( listener )`: Register a listener for size changes that affect the mobile breakpoint (<480px). Returns the unsubscribe function. +- `addIsDesktopListener( listener )`: Register a listener for size changes that affect the desktop breakpoint (>960px). Returns the unsubscribe function. - `getWindowInnerWidth()`: Get the inner width for the browser window. **Warning**: This method triggers a layout recalc, potentially resulting in performance issues. Please use a breakpoint instead wherever possible. ### Supported hooks diff --git a/client/lib/viewport/index.js b/client/lib/viewport/index.js index 1f9207edeef38c..f2df540cc7a735 100644 --- a/client/lib/viewport/index.js +++ b/client/lib/viewport/index.js @@ -116,7 +116,7 @@ export function isWithinBreakpoint( breakpoint ) { * @param {String} breakpoint The breakpoint to consider. * @param {Function} listener The listener to be called on change. * - * @returns {Function} The registered subscription; undefined if none. + * @returns {Function} The function to be called when unsubscribing. */ export function addWithinBreakpointListener( breakpoint, listener ) { if ( ! listener ) { @@ -128,29 +128,13 @@ export function addWithinBreakpointListener( breakpoint, listener ) { if ( mediaQueryList && ! isServer ) { const wrappedListener = evt => listener( evt.matches ); mediaQueryList.addListener( wrappedListener ); - return wrappedListener; + // Return unsubscribe function. + return () => mediaQueryList.removeListener( wrappedListener ); } return undefined; } -/** - * Removes a listener that was being notified of changes to breakpoint matching status. - * @param {String} breakpoint The breakpoint to consider. - * @param {Function} subscription The subscription to be removed. - */ -export function removeWithinBreakpointListener( breakpoint, subscription ) { - if ( ! subscription ) { - return; - } - - const mediaQueryList = getMediaQueryList( breakpoint ); - - if ( mediaQueryList && ! isServer ) { - mediaQueryList.removeListener( subscription ); - } -} - /** * Returns whether the current window width matches the mobile breakpoint. * @@ -170,14 +154,6 @@ export function addIsMobileListener( listener ) { return addWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); } -/** - * Removes a listener that was being notified of changes to mobile breakpoint matching status. - * @param {Function} listener The listener to be called on change. - */ -export function removeIsMobileListener( listener ) { - removeWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); -} - /** * Returns whether the current window width matches the desktop breakpoint. * @@ -197,14 +173,6 @@ export function addIsDesktopListener( listener ) { return addWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); } -/** - * Removes a listener that was being notified of changes to desktop breakpoint matching status. - * @param {Function} listener The listener to be called on change. - */ -export function removeIsDesktopListener( listener ) { - removeWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); -} - /** * Returns the current window width. * Avoid using this method, as it triggers a layout recalc. diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index 0ac334d5d0339d..3e9f74446d4f6e 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -11,7 +11,6 @@ import { camelCase, upperFirst } from 'lodash'; import { isWithinBreakpoint, addWithinBreakpointListener, - removeWithinBreakpointListener, MOBILE_BREAKPOINT, DESKTOP_BREAKPOINT, } from './index'; @@ -31,10 +30,12 @@ export function useBreakpoint( breakpoint ) { } useEffect(() => { - const subscription = addWithinBreakpointListener( breakpoint, handleBreakpointChange ); + const unsubscribe = addWithinBreakpointListener( breakpoint, handleBreakpointChange ); return function cleanup() { - removeWithinBreakpointListener( breakpoint, subscription ); + if ( unsubscribe ) { + unsubscribe(); + } }; }, []); diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js index 8577efae981f76..7bc6afec116097 100644 --- a/client/lib/viewport/test/index.js +++ b/client/lib/viewport/test/index.js @@ -90,9 +90,9 @@ describe( 'viewport', () => { test( 'should add a listener for a valid breakpoint', () => { const listener = jest.fn(); const event = { matches: 'bar' }; - let subscription; + let unsubscribe; expect( - () => ( subscription = viewport.addWithinBreakpointListener( '<960px', listener ) ) + () => ( unsubscribe = viewport.addWithinBreakpointListener( '<960px', listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( @@ -100,13 +100,15 @@ describe( 'viewport', () => { expect.any( Function ) ); // Call registered listener to make sure it got added correctly. - addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]( event ); + const registeredListener = + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + registeredListener( event ); expect( listener ).toHaveBeenCalledTimes( 1 ); expect( listener ).toHaveBeenCalledWith( 'bar' ); // Clean up. - try { - viewport.removeWithinBreakpointListener( '<960px', subscription ); - } catch {} + expect( () => unsubscribe() ).not.toThrow(); + expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 960px)', registeredListener ); } ); } ); @@ -118,21 +120,23 @@ describe( 'viewport', () => { test( 'should add a listener', () => { const listener = jest.fn(); const event = { matches: 'bar' }; - let subscription; - expect( () => ( subscription = viewport.addIsMobileListener( listener ) ) ).not.toThrow(); + let unsubscribe; + expect( () => ( unsubscribe = viewport.addIsMobileListener( listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', expect.any( Function ) ); // Call registered listener to make sure it got added correctly. - addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]( event ); + const registeredListener = + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + registeredListener( event ); expect( listener ).toHaveBeenCalledTimes( 1 ); expect( listener ).toHaveBeenCalledWith( 'bar' ); // Clean up. - try { - viewport.removeIsMobileListener( subscription ); - } catch {} + expect( () => unsubscribe() ).not.toThrow(); + expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', registeredListener ); } ); } ); @@ -144,85 +148,23 @@ describe( 'viewport', () => { test( 'should add a listener', () => { const listener = jest.fn(); const event = { matches: 'bar' }; - let subscription; - expect( () => ( subscription = viewport.addIsDesktopListener( listener ) ) ).not.toThrow(); + let unsubscribe; + expect( () => ( unsubscribe = viewport.addIsDesktopListener( listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', expect.any( Function ) ); // Call registered listener to make sure it got added correctly. - addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]( event ); + const registeredListener = + addListenerMock.mock.calls[ addListenerMock.mock.calls.length - 1 ][ 1 ]; + registeredListener( event ); expect( listener ).toHaveBeenCalledTimes( 1 ); expect( listener ).toHaveBeenCalledWith( 'bar' ); // Clean up. - try { - viewport.removeIsDesktopListener( subscription ); - } catch {} - } ); - } ); - - describe( 'removeWithinBreakpointListener', () => { - test( 'should do nothing if nothing is provided', () => { - expect( () => viewport.removeWithinBreakpointListener() ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); - } ); - test( 'should do nothing if an invalid breakpoint is provided', () => { - expect( () => viewport.removeWithinBreakpointListener( 'unknown', () => '' ) ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); - } ); - test( 'should do nothing if no listener is provided', () => { - expect( () => viewport.removeWithinBreakpointListener( '<960px' ) ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); - } ); - test( 'should not throw if an unregistered listener is provided', () => { - expect( () => viewport.removeWithinBreakpointListener( '<960px', () => '' ) ).not.toThrow(); - } ); - test( 'should remove an added listener', () => { - const listener = jest.fn(); - const subscription = viewport.addWithinBreakpointListener( '<960px', listener ); - - expect( () => - viewport.removeWithinBreakpointListener( '<960px', subscription ) - ).not.toThrow(); - expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); - expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 960px)', subscription ); - } ); - } ); - - describe( 'removeIsMobileListener', () => { - test( 'should do nothing if nothing is provided', () => { - expect( () => viewport.removeIsMobileListener() ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); - } ); - test( 'should not throw if an unregistered listener is provided', () => { - expect( () => viewport.removeIsMobileListener( () => '' ) ).not.toThrow(); - } ); - test( 'should remove an added listener', () => { - const listener = jest.fn(); - const subscription = viewport.addIsMobileListener( listener ); - - expect( () => viewport.removeIsMobileListener( subscription ) ).not.toThrow(); - expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); - expect( removeListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', subscription ); - } ); - } ); - - describe( 'removeIsDesktopListener', () => { - test( 'should do nothing if nothing is provided', () => { - expect( () => viewport.removeIsDesktopListener() ).not.toThrow(); - expect( removeListenerMock ).not.toHaveBeenCalled(); - } ); - test( 'should not throw if an unregistered listener is provided', () => { - expect( () => viewport.removeIsDesktopListener( () => '' ) ).not.toThrow(); - } ); - test( 'should remove an added listener', () => { - const listener = jest.fn(); - const subscription = viewport.addIsDesktopListener( listener ); - - expect( () => viewport.removeIsDesktopListener( subscription ) ).not.toThrow(); + expect( () => unsubscribe() ).not.toThrow(); expect( removeListenerMock ).toHaveBeenCalledTimes( 1 ); - expect( removeListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', subscription ); + expect( removeListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', registeredListener ); } ); } ); } ); From a4a1060d1f73d03590b0ff8246a7f4e81679a8f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Fri, 1 Mar 2019 14:42:12 +0000 Subject: [PATCH 13/23] Remove noop default from Tooltip. --- client/components/tooltip/index.jsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/client/components/tooltip/index.jsx b/client/components/tooltip/index.jsx index 01c0650a1cc9ef..e2111cfea1ec86 100644 --- a/client/components/tooltip/index.jsx +++ b/client/components/tooltip/index.jsx @@ -14,8 +14,6 @@ import classnames from 'classnames'; import Popover from 'components/popover'; import { useMobileBreakpoint } from 'lib/viewport/react-helpers'; -const noop = () => {}; - function Tooltip( props ) { const isMobile = useMobileBreakpoint(); @@ -39,7 +37,6 @@ function Tooltip( props ) { context={ props.context } id={ props.id } isVisible={ props.isVisible } - onClose={ noop } position={ props.position } showDelay={ props.showDelay } > From ac545b89f52bff654e69684ca7c0c7b40bf405d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Fri, 1 Mar 2019 14:58:54 +0000 Subject: [PATCH 14/23] Improvements to viewport hooks after review. --- client/lib/viewport/react-helpers.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index 3e9f74446d4f6e..9532de99c28b98 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -23,7 +23,7 @@ import { * @returns {Boolean} The current status for the breakpoint. */ export function useBreakpoint( breakpoint ) { - const [ isActive, setIsActive ] = useState( isWithinBreakpoint( breakpoint ) ); + const [ isActive, setIsActive ] = useState( () => isWithinBreakpoint( breakpoint ) ); function handleBreakpointChange( currentStatus ) { setIsActive( currentStatus ); @@ -37,7 +37,7 @@ export function useBreakpoint( breakpoint ) { unsubscribe(); } }; - }, []); + }, [ breakpoint ]); return isActive; } From 6fbfe0115c6f573dd9e1f6331ae0946fb9c5e8ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Fri, 1 Mar 2019 15:12:41 +0000 Subject: [PATCH 15/23] Change withBreakpoint API to add extra indirection. --- client/lib/viewport/README.md | 6 +++--- client/lib/viewport/react-helpers.js | 16 ++++++++-------- client/lib/viewport/test/react-helpers.js | 6 +++--- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/client/lib/viewport/README.md b/client/lib/viewport/README.md index 820547e0c8b03f..5c6507ecf15846 100644 --- a/client/lib/viewport/README.md +++ b/client/lib/viewport/README.md @@ -99,9 +99,9 @@ export default withMobileBreakpoint( MyComponent ); ### Supported higher-order components -- `withBreakpoint( WrappedComponent, breakpoint )`: Returns a wrapped component with the current status for a breakpoint as the `isBreakpointActive` prop. -- `useMobileBreakpoint( WrappedComponent )`: Returns a wrapped component with the current status for the mobile breakpoint as the `isBreakpointActive` prop. -- `useDesktopBreakpoint( WrappedComponent )`: Returns a wrapped component with the current status for the desktop breakpoint as the `isBreakpointActive` prop. +- `withBreakpoint( breakpoint )( WrappedComponent )`: Returns a wrapped component with the current status for a breakpoint as the `isBreakpointActive` prop. +- `withMobileBreakpoint( WrappedComponent )`: Returns a wrapped component with the current status for the mobile breakpoint as the `isBreakpointActive` prop. +- `withDesktopBreakpoint( WrappedComponent )`: Returns a wrapped component with the current status for the desktop breakpoint as the `isBreakpointActive` prop. ### Supported breakpoints diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index 9532de99c28b98..7b55793211c4f3 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -69,7 +69,7 @@ export function useDesktopBreakpoint() { * @param {String} breakpoint The breakpoint to consider. * @param {String} modifierName The name to modify the component with. * - * @returns {React.Component} The wrapped component. + * @returns {Function} The wrapped component. */ function withBreakpointAux( Wrapped, breakpoint, modifierName ) { const EnhancedComponent = function( props ) { @@ -77,7 +77,7 @@ function withBreakpointAux( Wrapped, breakpoint, modifierName ) { return ; }; - const { displayName = Wrapped.name || 'Component' } = Wrapped; + const displayName = Wrapped.displayName || Wrapped.name || 'Component'; EnhancedComponent.displayName = `${ upperFirst( camelCase( modifierName ) ) }(${ displayName })`; return EnhancedComponent; @@ -87,13 +87,13 @@ function withBreakpointAux( Wrapped, breakpoint, modifierName ) { * React higher order component for getting the status for a breakpoint and * keeping it updated. * - * @param {React.Component|Function} Wrapped The component to wrap. * @param {String} breakpoint The breakpoint to consider. * - * @returns {React.Component} The wrapped component. + * @returns {Function} A function that given a component returns the + * wrapped component. */ -export function withBreakpoint( Wrapped, breakpoint ) { - return withBreakpointAux( Wrapped, breakpoint, 'WithBreakpoint' ); +export function withBreakpoint( breakpoint ) { + return Wrapped => withBreakpointAux( Wrapped, breakpoint, 'WithBreakpoint' ); } /** @@ -102,7 +102,7 @@ export function withBreakpoint( Wrapped, breakpoint ) { * * @param {React.Component|Function} Wrapped The component to wrap. * - * @returns {React.Component} The wrapped component. + * @returns {Function} The wrapped component. */ export function withMobileBreakpoint( Wrapped ) { return withBreakpointAux( Wrapped, MOBILE_BREAKPOINT, 'WithMobileBreakpoint' ); @@ -114,7 +114,7 @@ export function withMobileBreakpoint( Wrapped ) { * * @param {React.Component|Function} Wrapped The component to wrap. * - * @returns {React.Component} The wrapped component. + * @returns {Function} The wrapped component. */ export function withDesktopBreakpoint( Wrapped ) { return withBreakpointAux( Wrapped, DESKTOP_BREAKPOINT, 'WithDesktopBreakpoint' ); diff --git a/client/lib/viewport/test/react-helpers.js b/client/lib/viewport/test/react-helpers.js index 8227d633664cc1..2266d8803f9ad2 100644 --- a/client/lib/viewport/test/react-helpers.js +++ b/client/lib/viewport/test/react-helpers.js @@ -177,7 +177,7 @@ describe( 'viewport/react-helpers', () => { } test( 'returns undefined when called with no breakpoint', () => { - const TestComponent = helpers.withBreakpoint( ExpectUndefinedComponent ); + const TestComponent = helpers.withBreakpoint()( ExpectUndefinedComponent ); act( () => { ReactDOM.render( , container ); @@ -187,7 +187,7 @@ describe( 'viewport/react-helpers', () => { } ); test( 'returns undefined for an unknown breakpoint', () => { - const TestComponent = helpers.withBreakpoint( ExpectUndefinedComponent, 'unknown' ); + const TestComponent = helpers.withBreakpoint( 'unknown' )( ExpectUndefinedComponent ); act( () => { ReactDOM.render( , container ); @@ -197,7 +197,7 @@ describe( 'viewport/react-helpers', () => { } ); test( 'returns the current breakpoint state for a valid breakpoint', () => { - const TestComponent = helpers.withBreakpoint( BaseComponent, '<960px' ); + const TestComponent = helpers.withBreakpoint( '<960px' )( BaseComponent ); runComponentTests( TestComponent, '(max-width: 960px)' ); } ); } ); From b1ca8784b13912496f65e2239c7c35df532c9a6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Fri, 1 Mar 2019 15:55:11 +0000 Subject: [PATCH 16/23] Further improvements to viewport API after review. --- client/lib/viewport/README.md | 6 +++--- client/lib/viewport/index.js | 16 ++++++++------ client/lib/viewport/react-helpers.js | 12 ++++------- client/lib/viewport/test/index.js | 32 ++++++++++++++++++---------- 4 files changed, 37 insertions(+), 29 deletions(-) diff --git a/client/lib/viewport/README.md b/client/lib/viewport/README.md index 5c6507ecf15846..21d7563b1a6b06 100644 --- a/client/lib/viewport/README.md +++ b/client/lib/viewport/README.md @@ -86,9 +86,9 @@ export default withMobileBreakpoint( MyComponent ); - `isWithinBreakpoint( breakpoint )`: Whether the current screen size matches the breakpoint. - `isMobile()`: Whether the current screen size matches a mobile breakpoint (<480px). - `isDesktop()`: Whether the current screen size matches a desktop breakpoint (>960px). -- `addWithinBreakpointListener( breakpoint, listener )`: Register a listener for size changes that affect the breakpoint. Returns the unsubscribe function. -- `addIsMobileListener( listener )`: Register a listener for size changes that affect the mobile breakpoint (<480px). Returns the unsubscribe function. -- `addIsDesktopListener( listener )`: Register a listener for size changes that affect the desktop breakpoint (>960px). Returns the unsubscribe function. +- `subscribeIsWithinBreakpoint( breakpoint, listener )`: Register a listener for size changes that affect the breakpoint. Returns the unsubscribe function. +- `subscribeIsMobile( listener )`: Register a listener for size changes that affect the mobile breakpoint (<480px). Returns the unsubscribe function. +- `subscribeIsDesktop( listener )`: Register a listener for size changes that affect the desktop breakpoint (>960px). Returns the unsubscribe function. - `getWindowInnerWidth()`: Get the inner width for the browser window. **Warning**: This method triggers a layout recalc, potentially resulting in performance issues. Please use a breakpoint instead wherever possible. ### Supported hooks diff --git a/client/lib/viewport/index.js b/client/lib/viewport/index.js index f2df540cc7a735..cfa0caebf46e82 100644 --- a/client/lib/viewport/index.js +++ b/client/lib/viewport/index.js @@ -46,6 +46,8 @@ export const DESKTOP_BREAKPOINT = '>960px'; const isServer = typeof window === 'undefined' || ! window.matchMedia; +const noop = () => null; + function createMediaQueryList( { min, max } = {} ) { if ( min !== undefined && max !== undefined ) { return isServer @@ -118,9 +120,9 @@ export function isWithinBreakpoint( breakpoint ) { * * @returns {Function} The function to be called when unsubscribing. */ -export function addWithinBreakpointListener( breakpoint, listener ) { +export function subscribeIsWithinBreakpoint( breakpoint, listener ) { if ( ! listener ) { - return; + return noop; } const mediaQueryList = getMediaQueryList( breakpoint ); @@ -132,7 +134,7 @@ export function addWithinBreakpointListener( breakpoint, listener ) { return () => mediaQueryList.removeListener( wrappedListener ); } - return undefined; + return noop; } /** @@ -150,8 +152,8 @@ export function isMobile() { * * @returns {Function} The registered subscription; undefined if none. */ -export function addIsMobileListener( listener ) { - return addWithinBreakpointListener( MOBILE_BREAKPOINT, listener ); +export function subscribeIsMobile( listener ) { + return subscribeIsWithinBreakpoint( MOBILE_BREAKPOINT, listener ); } /** @@ -169,8 +171,8 @@ export function isDesktop() { * * @returns {Function} The registered subscription; undefined if none. */ -export function addIsDesktopListener( listener ) { - return addWithinBreakpointListener( DESKTOP_BREAKPOINT, listener ); +export function subscribeIsDesktop( listener ) { + return subscribeIsWithinBreakpoint( DESKTOP_BREAKPOINT, listener ); } /** diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index 7b55793211c4f3..ad64caaf98fe1d 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -10,7 +10,7 @@ import { camelCase, upperFirst } from 'lodash'; */ import { isWithinBreakpoint, - addWithinBreakpointListener, + subscribeIsWithinBreakpoint, MOBILE_BREAKPOINT, DESKTOP_BREAKPOINT, } from './index'; @@ -30,13 +30,9 @@ export function useBreakpoint( breakpoint ) { } useEffect(() => { - const unsubscribe = addWithinBreakpointListener( breakpoint, handleBreakpointChange ); - - return function cleanup() { - if ( unsubscribe ) { - unsubscribe(); - } - }; + const unsubscribe = subscribeIsWithinBreakpoint( breakpoint, handleBreakpointChange ); + // The unsubscribe function is the entire cleanup for the effect. + return unsubscribe; }, [ breakpoint ]); return isActive; diff --git a/client/lib/viewport/test/index.js b/client/lib/viewport/test/index.js index 7bc6afec116097..0f05a1e5f00974 100644 --- a/client/lib/viewport/test/index.js +++ b/client/lib/viewport/test/index.js @@ -74,17 +74,27 @@ describe( 'viewport', () => { } ); } ); - describe( 'addWithinBreakpointListener', () => { + describe( 'subscribeIsWithinBreakpoint', () => { test( 'should do nothing if nothing is provided', () => { - expect( () => viewport.addWithinBreakpointListener() ).not.toThrow(); + let unsubscribe; + expect( () => ( unsubscribe = viewport.subscribeIsWithinBreakpoint() ) ).not.toThrow(); + expect( () => unsubscribe() ).not.toThrow(); expect( addListenerMock ).not.toHaveBeenCalled(); } ); test( 'should do nothing if an invalid breakpoint is provided', () => { - expect( () => viewport.addWithinBreakpointListener( 'unknown', () => '' ) ).not.toThrow(); + let unsubscribe; + expect( + () => ( unsubscribe = viewport.subscribeIsWithinBreakpoint( 'unknown', () => '' ) ) + ).not.toThrow(); + expect( () => unsubscribe() ).not.toThrow(); expect( addListenerMock ).not.toHaveBeenCalled(); } ); test( 'should do nothing if no listener is provided', () => { - expect( () => viewport.addWithinBreakpointListener( '<960px' ) ).not.toThrow(); + let unsubscribe; + expect( + () => ( unsubscribe = viewport.subscribeIsWithinBreakpoint( '<960px' ) ) + ).not.toThrow(); + expect( () => unsubscribe() ).not.toThrow(); expect( addListenerMock ).not.toHaveBeenCalled(); } ); test( 'should add a listener for a valid breakpoint', () => { @@ -92,7 +102,7 @@ describe( 'viewport', () => { const event = { matches: 'bar' }; let unsubscribe; expect( - () => ( unsubscribe = viewport.addWithinBreakpointListener( '<960px', listener ) ) + () => ( unsubscribe = viewport.subscribeIsWithinBreakpoint( '<960px', listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( @@ -112,16 +122,16 @@ describe( 'viewport', () => { } ); } ); - describe( 'addIsMobileListener', () => { + describe( 'subscribeIsMobile', () => { test( 'should do nothing if nothing is provided', () => { - expect( () => viewport.addIsMobileListener() ).not.toThrow(); + expect( () => viewport.subscribeIsMobile() ).not.toThrow(); expect( addListenerMock ).not.toHaveBeenCalled(); } ); test( 'should add a listener', () => { const listener = jest.fn(); const event = { matches: 'bar' }; let unsubscribe; - expect( () => ( unsubscribe = viewport.addIsMobileListener( listener ) ) ).not.toThrow(); + expect( () => ( unsubscribe = viewport.subscribeIsMobile( listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( '(max-width: 480px)', @@ -140,16 +150,16 @@ describe( 'viewport', () => { } ); } ); - describe( 'addIsDesktopListener', () => { + describe( 'subscribeIsDesktop', () => { test( 'should do nothing if nothing is provided', () => { - expect( () => viewport.addIsDesktopListener() ).not.toThrow(); + expect( () => viewport.subscribeIsDesktop() ).not.toThrow(); expect( addListenerMock ).not.toHaveBeenCalled(); } ); test( 'should add a listener', () => { const listener = jest.fn(); const event = { matches: 'bar' }; let unsubscribe; - expect( () => ( unsubscribe = viewport.addIsDesktopListener( listener ) ) ).not.toThrow(); + expect( () => ( unsubscribe = viewport.subscribeIsDesktop( listener ) ) ).not.toThrow(); expect( addListenerMock ).toHaveBeenCalledTimes( 1 ); expect( addListenerMock ).toHaveBeenCalledWith( '(min-width: 961px)', From b2ffccb59a589996b53ebb171d3e75fb7d63853e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Fri, 1 Mar 2019 16:39:53 +0000 Subject: [PATCH 17/23] Solve edge case with useBreakpoint when breakpoint changes. --- client/lib/viewport/react-helpers.js | 6 +++ client/lib/viewport/test/react-helpers.js | 52 ++++++++++++++++++++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index ad64caaf98fe1d..a18fe04cb82504 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -30,6 +30,12 @@ export function useBreakpoint( breakpoint ) { } useEffect(() => { + // Pick up changes to breakpoint, if any, and update state accordingly. + const currentStatus = isWithinBreakpoint( breakpoint ); + if ( isActive !== currentStatus ) { + handleBreakpointChange( currentStatus ); + } + const unsubscribe = subscribeIsWithinBreakpoint( breakpoint, handleBreakpointChange ); // The unsubscribe function is the entire cleanup for the effect. return unsubscribe; diff --git a/client/lib/viewport/test/react-helpers.js b/client/lib/viewport/test/react-helpers.js index 2266d8803f9ad2..b06e10a2c611a7 100644 --- a/client/lib/viewport/test/react-helpers.js +++ b/client/lib/viewport/test/react-helpers.js @@ -6,7 +6,7 @@ /** * Internal dependencies */ -import React from 'react'; +import React, { useState, useEffect } from 'react'; import ReactDOM from 'react-dom'; import { act } from 'react-dom/test-utils'; @@ -144,6 +144,56 @@ describe( 'viewport/react-helpers', () => { runComponentTests( TestComponent, '(max-width: 960px)' ); } ); + + test( 'correctly updates if the breakpoint changes', () => { + let callback; + + function TestComponent() { + const [ query, setQuery ] = useState( '<960px' ); + const isActive = helpers.useBreakpoint( query ); + const changeQuery = () => setQuery( '<480px' ); + + useEffect( () => { + callback = changeQuery; + } ); + + return isActive ? 'true' : 'false'; + } + + // Test initial state (defaults to true). + act( () => { + ReactDOM.render( +
+ +
, + container + ); + } ); + + expect( container.textContent ).toBe( 'true' ); + + // Change to false. + act( () => { + callQueryListeners( '(max-width: 960px)', false ); + } ); + + expect( container.textContent ).toBe( 'false' ); + + // Change breakpoint, defaulting back to true. + act( () => { + callback(); + } ); + expect( container.textContent ).toBe( 'true' ); + expect( listeners[ '(max-width: 960px)' ].length ).toBe( 0 ); + expect( listeners[ '(max-width: 480px)' ].length ).toBe( 1 ); + + // Ensure that listeners are cleaned up when the component unmounts. + act( () => { + ReactDOM.render(
, container ); + } ); + + expect( listeners[ '(max-width: 480px)' ].length ).toBe( 0 ); + } ); } ); describe( 'useMobileBreakpoint', () => { From 2e0a939283456e27879a84ee4f128a928c62176c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Mon, 4 Mar 2019 11:42:34 +0000 Subject: [PATCH 18/23] Fix some typos in lib/viewport README. --- client/lib/viewport/README.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/client/lib/viewport/README.md b/client/lib/viewport/README.md index 21d7563b1a6b06..d52e21a8b2e54c 100644 --- a/client/lib/viewport/README.md +++ b/client/lib/viewport/README.md @@ -12,7 +12,7 @@ import { isDesktop, isMobile } from 'lib/viewport'; if ( isDesktop() ) { // Render a component optimized for desktop view -} else ( isMobile() ) { +} else if ( isMobile() ) { // Render a component optimized for mobile view } ``` @@ -32,7 +32,7 @@ if ( isWithinBreakpoint( '>1400px' ) ) { Registering to listen to changes: ```js -import { addIsDesktopListener } from 'lib/viewport'; +import { subscribeIsDesktop } from 'lib/viewport'; class MyComponent extends React.Component { sizeChanged = matches => { @@ -42,13 +42,11 @@ class MyComponent extends React.Component { }; componentDidMount() { - this.unsubscribe = addIsDesktopListener( this.sizeChanged ); + this.unsubscribe = subscribeIsDesktop( this.sizeChanged ); } componentWillUnmount() { - if ( this.unsubscribe ) { - this.unsubscribe(); - } + this.unsubscribe(); } } ``` From 90b690108bc3eb20e84aa997b311dc7f41864f17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Mon, 4 Mar 2019 14:40:41 +0000 Subject: [PATCH 19/23] Tweak useBreakpoint again to ensure correctness at all times. Changing to a different breakpoint should now produce an immediate correct result, without needing to wait for an effect to fire. There should be no unnecessary re-renders either using this approach. --- client/lib/viewport/react-helpers.js | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index a18fe04cb82504..9ca8b5f9ab9193 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -23,17 +23,20 @@ import { * @returns {Boolean} The current status for the breakpoint. */ export function useBreakpoint( breakpoint ) { - const [ isActive, setIsActive ] = useState( () => isWithinBreakpoint( breakpoint ) ); - - function handleBreakpointChange( currentStatus ) { - setIsActive( currentStatus ); - } + const [ state, setState ] = useState( () => ( { + isActive: isWithinBreakpoint( breakpoint ), + breakpoint, + } ) ); useEffect(() => { - // Pick up changes to breakpoint, if any, and update state accordingly. - const currentStatus = isWithinBreakpoint( breakpoint ); - if ( isActive !== currentStatus ) { - handleBreakpointChange( currentStatus ); + function handleBreakpointChange( currentStatus ) { + setState( prevState => { + // Ensure we bail out without rendering if nothing changes, by preserving state. + if ( prevState.isActive === currentStatus && prevState.breakpoint === breakpoint ) { + return prevState; + } + return { isActive: currentStatus, breakpoint }; + } ); } const unsubscribe = subscribeIsWithinBreakpoint( breakpoint, handleBreakpointChange ); @@ -41,7 +44,7 @@ export function useBreakpoint( breakpoint ) { return unsubscribe; }, [ breakpoint ]); - return isActive; + return breakpoint === state.breakpoint ? state.isActive : isWithinBreakpoint( breakpoint ); } /** From 7488b2628a7f5b839782cc41a3771ea19c1786da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Mon, 4 Mar 2019 15:01:40 +0000 Subject: [PATCH 20/23] Rename a parameter in useBreakpoint. --- client/lib/viewport/react-helpers.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index 9ca8b5f9ab9193..e40eb166f86c34 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -29,13 +29,13 @@ export function useBreakpoint( breakpoint ) { } ) ); useEffect(() => { - function handleBreakpointChange( currentStatus ) { + function handleBreakpointChange( isActive ) { setState( prevState => { // Ensure we bail out without rendering if nothing changes, by preserving state. - if ( prevState.isActive === currentStatus && prevState.breakpoint === breakpoint ) { + if ( prevState.isActive === isActive && prevState.breakpoint === breakpoint ) { return prevState; } - return { isActive: currentStatus, breakpoint }; + return { isActive, breakpoint }; } ); } From a94f99e0beb02c2df6cf8573a19b664d169e4b9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Mon, 4 Mar 2019 15:10:28 +0000 Subject: [PATCH 21/23] Use createHigherOrderComponent for lib/viewport HOCs. --- client/lib/viewport/react-helpers.js | 56 ++++++++++++---------------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react-helpers.js index e40eb166f86c34..dbc9a44e018aeb 100644 --- a/client/lib/viewport/react-helpers.js +++ b/client/lib/viewport/react-helpers.js @@ -3,7 +3,7 @@ * External dependencies */ import React, { useState, useEffect } from 'react'; -import { camelCase, upperFirst } from 'lodash'; +import { createHigherOrderComponent } from '@wordpress/compose'; /** * Internal dependencies @@ -13,7 +13,7 @@ import { subscribeIsWithinBreakpoint, MOBILE_BREAKPOINT, DESKTOP_BREAKPOINT, -} from './index'; +} from '.'; /** * React hook for getting the status for a breakpoint and keeping it updated. @@ -67,27 +67,6 @@ export function useDesktopBreakpoint() { return useBreakpoint( DESKTOP_BREAKPOINT ); } -/** - * Auxiliary method to produce a higher order component for getting the status - * for a breakpoint and keeping it updated. It also sets the component name. - * @param {React.Component|Function} Wrapped The component to wrap. - * @param {String} breakpoint The breakpoint to consider. - * @param {String} modifierName The name to modify the component with. - * - * @returns {Function} The wrapped component. - */ -function withBreakpointAux( Wrapped, breakpoint, modifierName ) { - const EnhancedComponent = function( props ) { - const isActive = useBreakpoint( breakpoint ); - return ; - }; - - const displayName = Wrapped.displayName || Wrapped.name || 'Component'; - EnhancedComponent.displayName = `${ upperFirst( camelCase( modifierName ) ) }(${ displayName })`; - - return EnhancedComponent; -} - /** * React higher order component for getting the status for a breakpoint and * keeping it updated. @@ -97,9 +76,14 @@ function withBreakpointAux( Wrapped, breakpoint, modifierName ) { * @returns {Function} A function that given a component returns the * wrapped component. */ -export function withBreakpoint( breakpoint ) { - return Wrapped => withBreakpointAux( Wrapped, breakpoint, 'WithBreakpoint' ); -} +export const withBreakpoint = breakpoint => + createHigherOrderComponent( + WrappedComponent => props => { + const isBreakpointActive = useBreakpoint( breakpoint ); + return ; + }, + 'WithBreakpoint' + ); /** * React higher order component for getting the status for the mobile @@ -109,9 +93,13 @@ export function withBreakpoint( breakpoint ) { * * @returns {Function} The wrapped component. */ -export function withMobileBreakpoint( Wrapped ) { - return withBreakpointAux( Wrapped, MOBILE_BREAKPOINT, 'WithMobileBreakpoint' ); -} +export const withMobileBreakpoint = createHigherOrderComponent( + WrappedComponent => props => { + const isBreakpointActive = useBreakpoint( MOBILE_BREAKPOINT ); + return ; + }, + 'WithMobileBreakpoint' +); /** * React higher order component for getting the status for the desktop @@ -121,6 +109,10 @@ export function withMobileBreakpoint( Wrapped ) { * * @returns {Function} The wrapped component. */ -export function withDesktopBreakpoint( Wrapped ) { - return withBreakpointAux( Wrapped, DESKTOP_BREAKPOINT, 'WithDesktopBreakpoint' ); -} +export const withDesktopBreakpoint = createHigherOrderComponent( + WrappedComponent => props => { + const isBreakpointActive = useBreakpoint( DESKTOP_BREAKPOINT ); + return ; + }, + 'WithDesktopBreakpoint' +); From f63f191d26b17c4264fedfb921e4bd0db486c4e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Mon, 4 Mar 2019 15:14:21 +0000 Subject: [PATCH 22/23] Rename viewport/react-helpers to viewport/react. --- client/components/tooltip/index.jsx | 2 +- client/lib/viewport/README.md | 4 ++-- client/lib/viewport/{react-helpers.js => react.js} | 0 client/lib/viewport/test/{react-helpers.js => react.js} | 4 ++-- client/my-sites/activity/activity-log-item/aggregated.jsx | 2 +- client/my-sites/activity/activity-log-item/index.jsx | 2 +- client/my-sites/plan-features/item.jsx | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) rename client/lib/viewport/{react-helpers.js => react.js} (100%) rename client/lib/viewport/test/{react-helpers.js => react.js} (98%) diff --git a/client/components/tooltip/index.jsx b/client/components/tooltip/index.jsx index e2111cfea1ec86..e0049ca665c4ff 100644 --- a/client/components/tooltip/index.jsx +++ b/client/components/tooltip/index.jsx @@ -12,7 +12,7 @@ import classnames from 'classnames'; * Internal dependencies */ import Popover from 'components/popover'; -import { useMobileBreakpoint } from 'lib/viewport/react-helpers'; +import { useMobileBreakpoint } from 'lib/viewport/react'; function Tooltip( props ) { const isMobile = useMobileBreakpoint(); diff --git a/client/lib/viewport/README.md b/client/lib/viewport/README.md index d52e21a8b2e54c..7b7228a46a0404 100644 --- a/client/lib/viewport/README.md +++ b/client/lib/viewport/README.md @@ -56,7 +56,7 @@ It also comes with React helpers that help in registering a component to breakpo Using a hook: ```js -import { useMobileBreakpoint } from 'lib/viewport/react-helpers'; +import { useMobileBreakpoint } from 'lib/viewport/react'; export default function MyComponent( props ) { const isMobile = useMobileBreakpoint(); @@ -67,7 +67,7 @@ export default function MyComponent( props ) { Using a higher-order component: ```js -import { withMobileBreakpoint } from 'lib/viewport/react-helpers'; +import { withMobileBreakpoint } from 'lib/viewport/react'; class MyComponent extends React.Component { render() { diff --git a/client/lib/viewport/react-helpers.js b/client/lib/viewport/react.js similarity index 100% rename from client/lib/viewport/react-helpers.js rename to client/lib/viewport/react.js diff --git a/client/lib/viewport/test/react-helpers.js b/client/lib/viewport/test/react.js similarity index 98% rename from client/lib/viewport/test/react-helpers.js rename to client/lib/viewport/test/react.js index b06e10a2c611a7..659f2b3e72d1b4 100644 --- a/client/lib/viewport/test/react-helpers.js +++ b/client/lib/viewport/test/react.js @@ -46,7 +46,7 @@ const matchMediaMock = jest.fn( query => { return mediaListObjectMock; } ); -describe( 'viewport/react-helpers', () => { +describe( 'viewport/react', () => { let container; // Auxiliary method to test a valid component. @@ -93,7 +93,7 @@ describe( 'viewport/react-helpers', () => { beforeAll( async () => { window.matchMedia = matchMediaMock; - helpers = await import( '../react-helpers' ); + helpers = await import( '../react' ); // Disable console warnings. jest.spyOn( console, 'warn' ).mockImplementation( () => '' ); } ); diff --git a/client/my-sites/activity/activity-log-item/aggregated.jsx b/client/my-sites/activity/activity-log-item/aggregated.jsx index f1d64ae10de979..7e7b2541d70cf4 100644 --- a/client/my-sites/activity/activity-log-item/aggregated.jsx +++ b/client/my-sites/activity/activity-log-item/aggregated.jsx @@ -25,7 +25,7 @@ import { addQueryArgs } from 'lib/url'; import ActivityActor from './activity-actor'; import ActivityMedia from './activity-media'; import analytics from 'lib/analytics'; -import { withDesktopBreakpoint } from 'lib/viewport/react-helpers'; +import { withDesktopBreakpoint } from 'lib/viewport/react'; const MAX_STREAM_ITEMS_IN_AGGREGATE = 10; diff --git a/client/my-sites/activity/activity-log-item/index.jsx b/client/my-sites/activity/activity-log-item/index.jsx index 9c288d527710d3..22fe09be81f018 100644 --- a/client/my-sites/activity/activity-log-item/index.jsx +++ b/client/my-sites/activity/activity-log-item/index.jsx @@ -40,7 +40,7 @@ import getSiteGmtOffset from 'state/selectors/get-site-gmt-offset'; import getSiteTimezoneValue from 'state/selectors/get-site-timezone-value'; import { adjustMoment } from '../activity-log/utils'; import { getSite } from 'state/sites/selectors'; -import { withDesktopBreakpoint } from 'lib/viewport/react-helpers'; +import { withDesktopBreakpoint } from 'lib/viewport/react'; class ActivityLogItem extends Component { static propTypes = { diff --git a/client/my-sites/plan-features/item.jsx b/client/my-sites/plan-features/item.jsx index ed840a177b7a87..5979c1c1847cc3 100644 --- a/client/my-sites/plan-features/item.jsx +++ b/client/my-sites/plan-features/item.jsx @@ -11,7 +11,7 @@ import Gridicon from 'gridicons'; * Internal dependencies */ import InfoPopover from 'components/info-popover'; -import { useMobileBreakpoint } from 'lib/viewport/react-helpers'; +import { useMobileBreakpoint } from 'lib/viewport/react'; export default function PlanFeaturesItem( { children, description, hideInfoPopover } ) { const isMobile = useMobileBreakpoint(); From 32c4dfe932eb90d8486abcd015ef161e6ce7fdbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Gomes?= Date: Mon, 4 Mar 2019 15:21:44 +0000 Subject: [PATCH 23/23] Attach viewport react tests to the DOM. --- client/lib/viewport/test/react.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/client/lib/viewport/test/react.js b/client/lib/viewport/test/react.js index 659f2b3e72d1b4..49a2c50f12136a 100644 --- a/client/lib/viewport/test/react.js +++ b/client/lib/viewport/test/react.js @@ -100,11 +100,19 @@ describe( 'viewport/react', () => { beforeEach( () => { container = document.createElement( 'div' ); + document.body.appendChild( container ); + matchesMock.mockClear(); addListenerMock.mockClear(); removeListenerMock.mockClear(); } ); + afterEach( () => { + document.body.removeChild( container ); + ReactDOM.unmountComponentAtNode( container ); + container = null; + } ); + afterAll( () => { jest.restoreAllMocks(); } );