Plugins: Ensure sidebar plugins do not get auto-closed when opened on…

[gziolo]

… small screens

Description

Fixes #17623.

GIF:

Note how the Yoast pinned plugin sidebar is visible in the toolbar, but not clickable.

Even the item in the ellipsis menu doesn't invoke the sidebar.

As a reminder, pinned items are not visible on mobile (up to the 600px breakpoint), so all plugin sidebars should have a menu item so you can invoke them from there.

At the iPad breakpoint, the pinned item SHOULD be visible and SHOULD function, even if it opens the sidebar as a fullscreen modal interface.

I figured out what caused regression here:

gutenberg/packages/edit-post/src/components/editor-initialization/listener-hooks.js

Line 72 in a220eba

closeGeneralSidebar();

This forces the sidebar to get closed on small screens immediately after it gets opened.

Related PR which introduced the regression: #15444

[gziolo]

I added a new e2e test which covers the case when you open the plugin sidebar on medium screens.

There are 2 unit tests failing which still need to be investigated. See:

https://travis-ci.com/WordPress/gutenberg/jobs/241095619#L5659

FAIL packages/edit-post/src/components/editor-initialization/test/listener-hooks.js
  ● listener hook tests › useAdjustSidebarListener › closes sidebar if viewport is small and there is an active sidebar name available
    expect(jest.fn()).toHaveBeenCalledTimes(1)
    Expected mock function to have been called one time, but it was called zero times.
      150 | 			expect(
      151 | 				getSpyedFunction( STORE_KEY, 'closeGeneralSidebar' )
    > 152 | 			).toHaveBeenCalledTimes( 1 );
          | 			  ^
      153 | 		} );
      154 | 		it( 'opens sidebar if viewport is not small, and there is a cached sidebar to ' +
      155 | 			'reopen on expand', () => {
      at Object.toHaveBeenCalledTimes (packages/edit-post/src/components/editor-initialization/test/listener-hooks.js:152:6)
  ● listener hook tests › useAdjustSidebarListener › opens sidebar if viewport is not small, and there is a cached sidebar to reopen on expand
    expect(jest.fn()).toHaveBeenCalledWith(expected)
    Expected mock function to have been called with:
      ["foo"]
    But it was not called.
      165 | 			expect(
      166 | 				getSpyedFunction( STORE_KEY, 'openGeneralSidebar' )
    > 167 | 			).toHaveBeenCalledWith( 'foo' );
          | 			  ^
      168 | 			expect(
      169 | 				getSpyedFunction( STORE_KEY, 'openGeneralSidebar' )
      170 | 			).toHaveBeenCalledTimes( 1 );
      at Object.toHaveBeenCalledWith (packages/edit-post/src/components/editor-initialization/test/listener-hooks.js:167:6)

[nerrad]

I've updated the existing jest unit tests due to new expectations with the changes in this pull.

With the fix implemented in the tested hook, expectations 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.

[nerrad]

I added a few commits:

• fixing unit tests
• fixing a minor code style issue
• updating the changelog for the edit-post package.

Once tests pass this should be good to go.

[gziolo]

Thank you for all the help with fixing this regression. Great team work ❤️

Does it mean that I managed to tweak those refs and hooks properly? It’s something which felt a bit like a hack but I couldn’t think of s better way. I only know that refs are popular in this context.

[nerrad]

Does it mean that I managed to tweak those refs and hooks properly?

Looks fine to me 👍