From 8c0c7b7267473b5c197dd3052e9707f75f4e6448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20=28Greg=29=20Zi=C3=B3=C5=82kowski?= Date: Thu, 3 Oct 2019 09:53:03 +0200 Subject: [PATCH] =?UTF-8?q?Plugins:=20Ensure=20sidebar=20plugins=20do=20no?= =?UTF-8?q?t=20get=20auto-closed=20when=20opened=20on=E2=80=A6=20(#17712)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Plugins: Ensure sidebar plugins do not get auto-closed when opened on small screens * Tests: Add e2e test which ensures that the plugin sidebar gets operned on medium screens * update tests due to new expectations. With the fix implemented in the tested hook, expecations for initial state of the hook changes since the effect bails early when the current state of `isSmall` from the viewport store matches the previous state stored on the ref. On initial render, this hook will always bail early. I also added some inline comments to help explain what is happening in the test and what is expected. * fix code style issue * Add changelog entry for edit-post package --- .../src/set-browser-viewport.js | 1 + .../__snapshots__/plugins-api.test.js.snap | 2 + .../specs/plugins/plugins-api.test.js | 18 ++++++++ packages/edit-post/CHANGELOG.md | 6 +++ .../editor-initialization/listener-hooks.js | 28 +++++++----- .../test/listener-hooks.js | 44 ++++++++++++++++--- 6 files changed, 83 insertions(+), 16 deletions(-) diff --git a/packages/e2e-test-utils/src/set-browser-viewport.js b/packages/e2e-test-utils/src/set-browser-viewport.js index f6421215e00aa..b35eaa5891024 100644 --- a/packages/e2e-test-utils/src/set-browser-viewport.js +++ b/packages/e2e-test-utils/src/set-browser-viewport.js @@ -11,6 +11,7 @@ import { waitForWindowDimensions } from './wait-for-window-dimensions'; export async function setBrowserViewport( type ) { const allowedDimensions = { large: { width: 960, height: 700 }, + medium: { width: 768, height: 700 }, small: { width: 600, height: 700 }, }; const currentDimension = allowedDimensions[ type ]; diff --git a/packages/e2e-tests/specs/plugins/__snapshots__/plugins-api.test.js.snap b/packages/e2e-tests/specs/plugins/__snapshots__/plugins-api.test.js.snap index 2faff688fbd77..c2baa4270d086 100644 --- a/packages/e2e-tests/specs/plugins/__snapshots__/plugins-api.test.js.snap +++ b/packages/e2e-tests/specs/plugins/__snapshots__/plugins-api.test.js.snap @@ -2,4 +2,6 @@ exports[`Using Plugins API Document Setting Custom Panel Should render a custom panel inside Document Setting sidebar 1`] = `"My Custom Panel"`; +exports[`Using Plugins API Sidebar Medium screen Should open plugins sidebar using More Menu item and render content 1`] = `"
(no title)
Sidebar title plugin
"`; + exports[`Using Plugins API Sidebar Should open plugins sidebar using More Menu item and render content 1`] = `"
(no title)
Sidebar title plugin
"`; diff --git a/packages/e2e-tests/specs/plugins/plugins-api.test.js b/packages/e2e-tests/specs/plugins/plugins-api.test.js index 904fcaf0914e0..fedf3f25503c4 100644 --- a/packages/e2e-tests/specs/plugins/plugins-api.test.js +++ b/packages/e2e-tests/specs/plugins/plugins-api.test.js @@ -10,6 +10,7 @@ import { openDocumentSettingsSidebar, openPublishPanel, publishPost, + setBrowserViewport, } from '@wordpress/e2e-test-utils'; describe( 'Using Plugins API', () => { @@ -75,6 +76,23 @@ describe( 'Using Plugins API', () => { const pluginSidebarClosed = await page.$( '.edit-post-sidebar' ); expect( pluginSidebarClosed ).toBeNull(); } ); + + describe( 'Medium screen', () => { + beforeAll( async () => { + await setBrowserViewport( 'medium' ); + } ); + + afterAll( async () => { + await setBrowserViewport( 'large' ); + } ); + + it( 'Should open plugins sidebar using More Menu item and render content', async () => { + await clickOnMoreMenuItem( 'Sidebar title plugin' ); + + const pluginSidebarContent = await page.$eval( '.edit-post-sidebar', ( el ) => el.innerHTML ); + expect( pluginSidebarContent ).toMatchSnapshot(); + } ); + } ); } ); describe( 'Document Setting Custom Panel', () => { diff --git a/packages/edit-post/CHANGELOG.md b/packages/edit-post/CHANGELOG.md index 2a17afc9ad02b..69dd699b23426 100644 --- a/packages/edit-post/CHANGELOG.md +++ b/packages/edit-post/CHANGELOG.md @@ -1,3 +1,9 @@ +## Master + +### Bug Fixes + +- Fix regression introduced by EditorInitializer component which auto-closed sidebar plugins when opened on small screens. ([#17712](https://github.com/WordPress/gutenberg/pull/17712)) + ## 3.6.0 (2019-08-05) ### Refactor diff --git a/packages/edit-post/src/components/editor-initialization/listener-hooks.js b/packages/edit-post/src/components/editor-initialization/listener-hooks.js index 56d7761270eac..51b1b2920e67b 100644 --- a/packages/edit-post/src/components/editor-initialization/listener-hooks.js +++ b/packages/edit-post/src/components/editor-initialization/listener-hooks.js @@ -54,27 +54,35 @@ export const useBlockSelectionListener = ( postId ) => { * @param {number} postId The current post id. */ export const useAdjustSidebarListener = ( postId ) => { - const { isSmall, sidebarToReOpenOnExpand } = useSelect( + const { isSmall, activeGeneralSidebarName } = useSelect( ( select ) => ( { isSmall: select( 'core/viewport' ).isViewportMatch( '< medium' ), - sidebarToReOpenOnExpand: select( STORE_KEY ).getActiveGeneralSidebarName(), + activeGeneralSidebarName: select( STORE_KEY ).getActiveGeneralSidebarName(), } ), [ postId ] ); const { openGeneralSidebar, closeGeneralSidebar } = useDispatch( STORE_KEY ); - const previousOpenedSidebar = useRef( '' ); + const previousIsSmall = useRef( isSmall ); + const sidebarToReOpenOnExpand = useRef( null ); useEffect( () => { - if ( isSmall && sidebarToReOpenOnExpand ) { - previousOpenedSidebar.current = sidebarToReOpenOnExpand; - closeGeneralSidebar(); - } else if ( ! isSmall && previousOpenedSidebar.current ) { - openGeneralSidebar( previousOpenedSidebar.current ); - previousOpenedSidebar.current = ''; + if ( previousIsSmall.current === isSmall ) { + return; + } + previousIsSmall.current = isSmall; + + if ( isSmall ) { + sidebarToReOpenOnExpand.current = activeGeneralSidebarName; + if ( activeGeneralSidebarName ) { + closeGeneralSidebar(); + } + } else if ( sidebarToReOpenOnExpand.current && ! activeGeneralSidebarName ) { + openGeneralSidebar( sidebarToReOpenOnExpand.current ); + sidebarToReOpenOnExpand.current = null; } - }, [ isSmall, sidebarToReOpenOnExpand ] ); + }, [ isSmall, activeGeneralSidebarName ] ); }; /** diff --git a/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js b/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js index 350a23f5b0ca3..87c2bc616e0bf 100644 --- a/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js +++ b/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js @@ -137,10 +137,11 @@ describe( 'listener hook tests', () => { getSpyedFunction( STORE_KEY, 'closeGeneralSidebar' ) ).not.toHaveBeenCalled(); } ); - it( 'closes sidebar if viewport is small and there is an active ' + - 'sidebar name available', () => { + it( 'does not close sidebar if viewport is small and there is an active ' + + 'sidebar name available on initial render', () => { setMockReturnValue( 'core/viewport', 'isViewportMatch', true ); setMockReturnValue( STORE_KEY, 'getActiveGeneralSidebarName', 'foo' ); + // initial render does nothing (and sidebar will be closed already) act( () => { renderComponent( useAdjustSidebarListener, 10 ); } ); @@ -149,22 +150,53 @@ describe( 'listener hook tests', () => { ).not.toHaveBeenCalled(); expect( getSpyedFunction( STORE_KEY, 'closeGeneralSidebar' ) - ).toHaveBeenCalledTimes( 1 ); + ).not.toHaveBeenCalled(); } ); - it( 'opens sidebar if viewport is not small, and there is a cached sidebar to ' + - 'reopen on expand', () => { + it( 'closes sidebar if viewport is small and there is an active ' + + 'sidebar name available when viewport size changes', () => { + setMockReturnValue( 'core/viewport', 'isViewportMatch', false ); + setMockReturnValue( STORE_KEY, 'getActiveGeneralSidebarName', 'foo' ); + // initial render does nothing and sidebar will be open already. + act( () => { + renderComponent( useAdjustSidebarListener, 10 ); + } ); setMockReturnValue( 'core/viewport', 'isViewportMatch', true ); + // This render should result in the sidebar closing because viewport is + // now small triggering a change. + act( () => { + subscribeTrigger(); + } ); + expect( + getSpyedFunction( STORE_KEY, 'openGeneralSidebar' ) + ).not.toHaveBeenCalled(); + expect( + getSpyedFunction( STORE_KEY, 'closeGeneralSidebar' ) + ).toHaveBeenCalledTimes( 1 ); + } ); + it( 'opens sidebar if viewport is not small, and there is a cached sidebar ' + + 'to reopen on expand', () => { + setMockReturnValue( 'core/viewport', 'isViewportMatch', false ); setMockReturnValue( STORE_KEY, 'getActiveGeneralSidebarName', 'foo' ); + // initial render does nothing and sidebar should be open. act( () => { renderComponent( useAdjustSidebarListener, 10 ); } ); + setMockReturnValue( 'core/viewport', 'isViewportMatch', true ); + setMockReturnValue( STORE_KEY, 'getActiveGeneralSidebarName', 'bar' ); + // next render should close the sidebar and active sidebar at time of + // closing is cached. + act( () => { + subscribeTrigger(); + } ); setMockReturnValue( 'core/viewport', 'isViewportMatch', false ); + setMockReturnValue( STORE_KEY, 'getActiveGeneralSidebarName', '' ); + // next render should open the sidebar to the cached general sidebar name. act( () => { subscribeTrigger(); } ); expect( getSpyedFunction( STORE_KEY, 'openGeneralSidebar' ) - ).toHaveBeenCalledWith( 'foo' ); + ).toHaveBeenCalledWith( 'bar' ); expect( getSpyedFunction( STORE_KEY, 'openGeneralSidebar' ) ).toHaveBeenCalledTimes( 1 );