Skip to content

Commit

Permalink
Components: Restore click-to-close behavior of Dropdown toggle (#11633)
Browse files Browse the repository at this point in the history
* 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

* Revert "Components: Remove redundant onClickOutside handler from Dropdown (#11253)"

This reverts commit 58725c4.

* Testing: Verify Popover toggle behavior

* Components: Document Dropdown click outside behavior

* Components: Convert Dropdown container to use createRef
  • Loading branch information
aduth authored Nov 8, 2018
1 parent 005c790 commit a0d0e1a
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 2 deletions.
19 changes: 18 additions & 1 deletion packages/components/src/dropdown/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ class Dropdown extends Component {

this.toggle = this.toggle.bind( this );
this.close = this.close.bind( this );
this.closeIfClickOutside = this.closeIfClickOutside.bind( this );
this.refresh = this.refresh.bind( this );

this.containerRef = createRef();
this.popoverRef = createRef();

this.state = {
Expand Down Expand Up @@ -56,6 +58,20 @@ class Dropdown extends Component {
} ) );
}

/**
* 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 ) {
if ( ! this.containerRef.current.contains( event.target ) ) {
this.close();
}
}

close() {
this.setState( { isOpen: false } );
}
Expand All @@ -75,14 +91,15 @@ class Dropdown extends Component {
const args = { isOpen, onToggle: this.toggle, onClose: this.close };

return (
<div className={ className }>
<div className={ className } ref={ this.containerRef }>
{ renderToggle( args ) }
{ isOpen && (
<Popover
className={ contentClassName }
ref={ this.popoverRef }
position={ position }
onClose={ this.close }
onClickOutside={ this.closeIfClickOutside }
expandOnMobile={ expandOnMobile }
headerTitle={ headerTitle }
>
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/demo.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
} );
} );
26 changes: 26 additions & 0 deletions test/e2e/specs/popovers.test.js
Original file line number Diff line number Diff line change
@@ -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 );
} );
} );
} );

0 comments on commit a0d0e1a

Please sign in to comment.