From 3f6fd052be6d6c59a7afa246c64ef111c2f67f70 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 8 Nov 2018 10:48:52 -0500 Subject: [PATCH 1/5] Testing: Correct truthy test of immediately saveable demo Previously assumed the selector would throw if not found, but in-fact returns `null`. See: https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pageselector --- test/e2e/specs/demo.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/specs/demo.test.js b/test/e2e/specs/demo.test.js index bcf0987ae8771..cb8ef8b85dc40 100644 --- a/test/e2e/specs/demo.test.js +++ b/test/e2e/specs/demo.test.js @@ -40,6 +40,6 @@ describe( 'new editor state', () => { } ); it( 'should be immediately saveable', async () => { - await page.$( 'button.editor-post-save-draft' ); + expect( await page.$( 'button.editor-post-save-draft' ) ).toBeTruthy(); } ); } ); From 72ae189dfb417020e6313763d804c0319ba5213f Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 8 Nov 2018 10:49:17 -0500 Subject: [PATCH 2/5] Revert "Components: Remove redundant onClickOutside handler from Dropdown (#11253)" This reverts commit 58725c42980f81dbcf18d921be7ca43507c968e0. --- packages/components/src/dropdown/index.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/components/src/dropdown/index.js b/packages/components/src/dropdown/index.js index c99c0c3e8ea47..c8ba61f4ad183 100644 --- a/packages/components/src/dropdown/index.js +++ b/packages/components/src/dropdown/index.js @@ -11,13 +11,12 @@ import Popover from '../popover'; class Dropdown extends Component { constructor() { super( ...arguments ); - this.toggle = this.toggle.bind( this ); this.close = this.close.bind( this ); + this.clickOutside = this.clickOutside.bind( this ); + this.bindContainer = this.bindContainer.bind( this ); this.refresh = this.refresh.bind( this ); - this.popoverRef = createRef(); - this.state = { isOpen: false, }; @@ -39,6 +38,10 @@ class Dropdown extends Component { } } + bindContainer( ref ) { + this.container = ref; + } + /** * When contents change height due to user interaction, * `refresh` can be called to re-render Popover with correct @@ -56,6 +59,12 @@ class Dropdown extends Component { } ) ); } + clickOutside( event ) { + if ( ! this.container.contains( event.target ) ) { + this.close(); + } + } + close() { this.setState( { isOpen: false } ); } @@ -75,7 +84,7 @@ class Dropdown extends Component { const args = { isOpen, onToggle: this.toggle, onClose: this.close }; return ( -
+
{ renderToggle( args ) } { isOpen && ( From ca068167bc65baf5fb1fe40517a8b539ae78f35b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 8 Nov 2018 10:49:23 -0500 Subject: [PATCH 3/5] Testing: Verify Popover toggle behavior --- test/e2e/specs/popovers.test.js | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 test/e2e/specs/popovers.test.js diff --git a/test/e2e/specs/popovers.test.js b/test/e2e/specs/popovers.test.js new file mode 100644 index 0000000000000..202c8d7cb9362 --- /dev/null +++ b/test/e2e/specs/popovers.test.js @@ -0,0 +1,26 @@ +/** + * Internal dependencies + */ +import { newPost } from '../support/utils'; + +describe( 'popovers', () => { + beforeEach( async () => { + await newPost(); + } ); + + describe( 'dropdown', () => { + it( 'toggles via click', async () => { + const isMoreMenuOpen = async () => !! await page.$( '.edit-post-more-menu__content' ); + + expect( await isMoreMenuOpen() ).toBe( false ); + + // Toggle opened. + await page.click( '.edit-post-more-menu > button' ); + expect( await isMoreMenuOpen() ).toBe( true ); + + // Toggle closed. + await page.click( '.edit-post-more-menu > button' ); + expect( await isMoreMenuOpen() ).toBe( false ); + } ); + } ); +} ); From e77207383f839fd98cd628e94ede69f6fd95d04e Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 8 Nov 2018 10:54:28 -0500 Subject: [PATCH 4/5] Components: Document Dropdown click outside behavior --- packages/components/src/dropdown/index.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/components/src/dropdown/index.js b/packages/components/src/dropdown/index.js index c8ba61f4ad183..1ba507a7b0f39 100644 --- a/packages/components/src/dropdown/index.js +++ b/packages/components/src/dropdown/index.js @@ -13,8 +13,8 @@ class Dropdown extends Component { super( ...arguments ); this.toggle = this.toggle.bind( this ); this.close = this.close.bind( this ); - this.clickOutside = this.clickOutside.bind( this ); this.bindContainer = this.bindContainer.bind( this ); + this.closeIfClickOutside = this.closeIfClickOutside.bind( this ); this.refresh = this.refresh.bind( this ); this.popoverRef = createRef(); this.state = { @@ -59,8 +59,15 @@ class Dropdown extends Component { } ) ); } - clickOutside( event ) { - if ( ! this.container.contains( event.target ) ) { + /** + * Closes the dropdown if a click occurs outside the dropdown wrapper. This + * is intentionally distinct from `onClose` in that a click outside the + * popover may occur in the toggling of the dropdown via its toggle button. + * The correct behavior is to keep the dropdown closed. + * + * @param {MouseEvent} event Click event triggering `onClickOutside`. + */ + closeIfClickOutside( event ) { this.close(); } } @@ -92,7 +99,7 @@ class Dropdown extends Component { ref={ this.popoverRef } position={ position } onClose={ this.close } - onClickOutside={ this.clickOutside } + onClickOutside={ this.closeIfClickOutside } expandOnMobile={ expandOnMobile } headerTitle={ headerTitle } > From 2211abb3f1e08231ad01d6c23346aeec569b7e22 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 8 Nov 2018 10:54:57 -0500 Subject: [PATCH 5/5] Components: Convert Dropdown container to use createRef --- packages/components/src/dropdown/index.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/components/src/dropdown/index.js b/packages/components/src/dropdown/index.js index 1ba507a7b0f39..29b7700a0e636 100644 --- a/packages/components/src/dropdown/index.js +++ b/packages/components/src/dropdown/index.js @@ -11,12 +11,15 @@ import Popover from '../popover'; class Dropdown extends Component { constructor() { super( ...arguments ); + this.toggle = this.toggle.bind( this ); this.close = this.close.bind( this ); - this.bindContainer = this.bindContainer.bind( this ); this.closeIfClickOutside = this.closeIfClickOutside.bind( this ); this.refresh = this.refresh.bind( this ); + + this.containerRef = createRef(); this.popoverRef = createRef(); + this.state = { isOpen: false, }; @@ -38,10 +41,6 @@ class Dropdown extends Component { } } - bindContainer( ref ) { - this.container = ref; - } - /** * When contents change height due to user interaction, * `refresh` can be called to re-render Popover with correct @@ -68,6 +67,7 @@ class Dropdown extends Component { * @param {MouseEvent} event Click event triggering `onClickOutside`. */ closeIfClickOutside( event ) { + if ( ! this.containerRef.current.contains( event.target ) ) { this.close(); } } @@ -91,7 +91,7 @@ class Dropdown extends Component { const args = { isOpen, onToggle: this.toggle, onClose: this.close }; return ( -
+
{ renderToggle( args ) } { isOpen && (