Components: Remove Popover isOpen prop, render by presence (#5015)

* Components: Conditionally render popover * Components: Remove redundant Dropdown FocusManaged Already applied by Popover in all cases except when focusOnMount is false * Components: Remove unused FocusManaged
WordPress · Feb 13, 2018 · f836938 · f836938
1 parent a345029
commit f836938
Show file tree

Hide file tree

Showing 10 changed files with 138 additions and 165 deletions.
diff --git a/components/autocomplete/index.js b/components/autocomplete/index.js
@@ -472,35 +472,36 @@ export class Autocomplete extends Component {
 				className="components-autocomplete"
 			>
 				{ children( { isExpanded, listBoxId, activeId } ) }
-				<Popover
-					isOpen={ isExpanded }
-					focusOnOpen={ false }
-					onClose={ this.reset }
-					position="top right"
-					className="components-autocomplete__popover"
-					getAnchorRect={ this.getWordRect }
-				>
-					<div
-						id={ listBoxId }
-						role="listbox"
-						className="components-autocomplete__results"
+				{ isExpanded && (
+					<Popover
+						focusOnMount={ false }
+						onClose={ this.reset }
+						position="top right"
+						className="components-autocomplete__popover"
+						getAnchorRect={ this.getWordRect }
 					>
-						{ isExpanded && map( filteredOptions, ( option, index ) => (
-							<Button
-								key={ option.key }
-								id={ `components-autocomplete-item-${ instanceId }-${ option.key }` }
-								role="option"
-								aria-selected={ index === selectedIndex }
-								className={ classnames( 'components-autocomplete__result', className, {
-									'is-selected': index === selectedIndex,
-								} ) }
-								onClick={ () => this.select( option ) }
-							>
-								{ option.label }
-							</Button>
-						) ) }
-					</div>
-				</Popover>
+						<div
+							id={ listBoxId }
+							role="listbox"
+							className="components-autocomplete__results"
+						>
+							{ isExpanded && map( filteredOptions, ( option, index ) => (
+								<Button
+									key={ option.key }
+									id={ `components-autocomplete-item-${ instanceId }-${ option.key }` }
+									role="option"
+									aria-selected={ index === selectedIndex }
+									className={ classnames( 'components-autocomplete__result', className, {
+										'is-selected': index === selectedIndex,
+									} ) }
+									onClick={ () => this.select( option ) }
+								>
+									{ option.label }
+								</Button>
+							) ) }
+						</div>
+					</Popover>
+				) }
 			</div>
 		);
 		/* eslint-enable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */

diff --git a/components/autocomplete/test/index.js b/components/autocomplete/test/index.js
@@ -127,7 +127,7 @@ function expectInitialState( wrapper ) {
 	expect( wrapper.state( 'query' ) ).toBeUndefined();
 	expect( wrapper.state( 'search' ) ).toEqual( /./ );
 	expect( wrapper.state( 'filteredOptions' ) ).toEqual( [] );
-	expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( false );
+	expect( wrapper.find( 'Popover' ) ).toHaveLength( 0 );
 	expect( wrapper.find( '.components-autocomplete__result' ) ).toHaveLength( 0 );
 }
 
@@ -206,7 +206,6 @@ describe( 'Autocomplete', () => {
 		it( 'renders children', () => {
 			const wrapper = makeAutocompleter( [] );
 			expect( wrapper.state().open ).toBeUndefined();
-			expect( wrapper.find( 'Popover' ).prop( 'focusOnOpen' ) ).toBe( false );
 			expect( wrapper.childAt( 0 ).hasClass( 'components-autocomplete' ) ).toBe( true );
 			expect( wrapper.find( '.fake-editor' ) ).toHaveLength( 1 );
 		} );
@@ -226,7 +225,8 @@ describe( 'Autocomplete', () => {
 				expect( wrapper.state( 'filteredOptions' ) ).toEqual( [
 					{ key: '0-0', value: 1, label: 'Bananas', keywords: [ 'fruit' ] },
 				] );
-				expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( true );
+				expect( wrapper.find( 'Popover' ) ).toHaveLength( 1 );
+				expect( wrapper.find( 'Popover' ).prop( 'focusOnMount' ) ).toBe( false );
 				expect( wrapper.find( 'button.components-autocomplete__result' ) ).toHaveLength( 1 );
 				done();
 			} );
@@ -245,7 +245,7 @@ describe( 'Autocomplete', () => {
 				expect( wrapper.state( 'query' ) ).toEqual( 'zzz' );
 				expect( wrapper.state( 'search' ) ).toEqual( /(?:\b|\s|^)zzz/i );
 				expect( wrapper.state( 'filteredOptions' ) ).toEqual( [] );
-				expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( false );
+				expect( wrapper.find( 'Popover' ) ).toHaveLength( 0 );
 				expect( wrapper.find( 'button.components-autocomplete__result' ) ).toHaveLength( 0 );
 				done();
 			} );
@@ -283,7 +283,7 @@ describe( 'Autocomplete', () => {
 					{ key: '0-1', value: 2, label: 'Apple', keywords: [ 'fruit' ] },
 					{ key: '0-2', value: 3, label: 'Avocado', keywords: [ 'fruit' ] },
 				] );
-				expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( true );
+				expect( wrapper.find( 'Popover' ) ).toHaveLength( 1 );
 				expect( wrapper.find( 'button.components-autocomplete__result' ) ).toHaveLength( 3 );
 				done();
 			} );
@@ -307,7 +307,7 @@ describe( 'Autocomplete', () => {
 					{ key: '0-1', value: 2, label: 'Apple', keywords: [ 'fruit' ] },
 					{ key: '0-2', value: 3, label: 'Avocado', keywords: [ 'fruit' ] },
 				] );
-				expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( true );
+				expect( wrapper.find( 'Popover' ) ).toHaveLength( 1 );
 				expect( wrapper.find( 'button.components-autocomplete__result' ) ).toHaveLength( 3 );
 				done();
 			} );
@@ -330,7 +330,7 @@ describe( 'Autocomplete', () => {
 					{ key: '0-1', value: 2, label: 'Apple', keywords: [ 'fruit' ] },
 					{ key: '0-2', value: 3, label: 'Avocado', keywords: [ 'fruit' ] },
 				] );
-				expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( true );
+				expect( wrapper.find( 'Popover' ) ).toHaveLength( 1 );
 				expect( wrapper.find( 'button.components-autocomplete__result' ) ).toHaveLength( 2 );
 				// simulate typing 'p'
 				simulateInput( wrapper, [ tx( 'ap' ) ] );
@@ -342,7 +342,7 @@ describe( 'Autocomplete', () => {
 				expect( wrapper.state( 'filteredOptions' ) ).toEqual( [
 					{ key: '0-1', value: 2, label: 'Apple', keywords: [ 'fruit' ] },
 				] );
-				expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( true );
+				expect( wrapper.find( 'Popover' ) ).toHaveLength( 1 );
 				expect( wrapper.find( 'button.components-autocomplete__result' ) ).toHaveLength( 1 );
 				// simulate typing ' '
 				simulateInput( wrapper, [ tx( 'ap ' ) ] );
@@ -445,7 +445,7 @@ describe( 'Autocomplete', () => {
 					{ key: '0-1', value: 2, label: 'Apple', keywords: [ 'fruit' ] },
 					{ key: '0-2', value: 3, label: 'Avocado', keywords: [ 'fruit' ] },
 				] );
-				expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( false );
+				expect( wrapper.find( 'Popover' ) ).toHaveLength( 0 );
 				// the editor should not have gotten the event
 				expect( editorKeydown ).not.toHaveBeenCalled();
 				done();

diff --git a/components/dropdown-menu/test/index.js b/components/dropdown-menu/test/index.js
@@ -65,9 +65,7 @@ describe( 'DropdownMenu', () => {
 				keyCode: DOWN,
 			} );
 
-			const popover = wrapper.find( 'Popover' );
-
-			expect( popover.prop( 'isOpen' ) ).toBe( true );
+			expect( wrapper.find( 'Popover' ) ).toHaveLength( 1 );
 		} );
 	} );
 } );
diff --git a/components/dropdown/index.js b/components/dropdown/index.js
@@ -6,11 +6,8 @@ import { Component } from '@wordpress/element';
 /**
  * Internal Dependencies
  */
-import withFocusReturn from '../higher-order/with-focus-return';
 import Popover from '../popover';
 
-const FocusManaged = withFocusReturn( ( { children } ) => children );
-
 class Dropdown extends Component {
 	constructor() {
 		super( ...arguments );
@@ -80,18 +77,17 @@ class Dropdown extends Component {
 				   */ }
 				<div>
 					{ renderToggle( args ) }
-					<Popover
-						className={ contentClassName }
-						isOpen={ isOpen }
-						position={ position }
-						onClose={ this.close }
-						onClickOutside={ this.clickOutside }
-						expandOnMobile={ expandOnMobile }
-					>
-						<FocusManaged>
+					{ isOpen && (
+						<Popover
+							className={ contentClassName }
+							position={ position }
+							onClose={ this.close }
+							onClickOutside={ this.clickOutside }
+							expandOnMobile={ expandOnMobile }
+						>
 							{ renderContent( args ) }
-						</FocusManaged>
-					</Popover>
+						</Popover>
+					) }
 				</div>
 			</div>
 		);

diff --git a/components/dropdown/test/index.js b/components/dropdown/test/index.js
@@ -9,7 +9,7 @@ import { mount } from 'enzyme';
 import Dropdown from '../';
 
 describe( 'Dropdown', () => {
-	const expectPopoverOpened = ( wrapper, opened ) => expect( wrapper.find( 'Popover' ) ).toHaveProp( 'isOpen', opened );
+	const expectPopoverVisible = ( wrapper, visible ) => expect( wrapper.find( 'Popover' ) ).toHaveLength( visible ? 1 : 0 );
 
 	it( 'should toggle the dropdown properly', () => {
 		const expectButtonExpanded = ( wrapper, expanded ) => {
@@ -26,13 +26,13 @@ describe( 'Dropdown', () => {
 		/> );
 
 		expectButtonExpanded( wrapper, false );
-		expectPopoverOpened( wrapper, false );
+		expectPopoverVisible( wrapper, false );
 
 		wrapper.find( 'button' ).simulate( 'click' );
 		wrapper.update();
 
 		expectButtonExpanded( wrapper, true );
-		expectPopoverOpened( wrapper, true );
+		expectPopoverVisible( wrapper, true );
 	} );
 
 	it( 'should close the dropdown when calling onClose', () => {
@@ -46,16 +46,16 @@ describe( 'Dropdown', () => {
 			renderContent={ () => null }
 		/> );
 
-		expectPopoverOpened( wrapper, false );
+		expectPopoverVisible( wrapper, false );
 
 		wrapper.find( '.open' ).simulate( 'click' );
 		wrapper.update();
 
-		expectPopoverOpened( wrapper, true );
+		expectPopoverVisible( wrapper, true );
 
 		wrapper.find( '.close' ).simulate( 'click' );
 		wrapper.update();
 
-		expectPopoverOpened( wrapper, false );
+		expectPopoverVisible( wrapper, false );
 	} );
 } );
diff --git a/components/popover/README.md b/components/popover/README.md
@@ -14,18 +14,21 @@ function ToggleButton( { isVisible, toggleVisible } ) {
 	return (
 		<button onClick={ toggleVisible }>
 			Toggle Popover!
-			<Popover
-				isOpen={ isVisible }
-				onClose={ toggleVisible }
-				onClick={ ( event ) => event.stopPropagation() }
-			>
-				Popover is toggled!
-			</Popover>
+			{ isVisible && (
+				<Popover
+					onClose={ toggleVisible }
+					onClick={ ( event ) => event.stopPropagation() }
+				>
+					Popover is toggled!
+				</Popover>
+			) }
 		</button>
 	);
 }
 ```
 
+If a Popover is returned by your component, it will be shown. To hide the popover, simply omit it from your component's render value.
+
 If you want Popover elementss to render to a specific location on the page to allow style cascade to take effect, you must render a `Popover.Slot` further up the element tree:
 
 ```jsx
@@ -48,17 +51,9 @@ render(
 
 The component accepts the following props. Props not included in this set will be applied to the element wrapping Popover content.
 
-### isOpen
-
-As a controlled component, it is expected that you will pass `isOpen` to control whether the popover is visible. Refer to the `onClose` documentation for the complementary behavior for determining when this value should be toggled in your parent component state.
-
-- Type: `Boolean`
-- Required: No
-- Default: `false`
-
-### focusOnOpen
+### focusOnMount
 
-By default, the popover will receive focus when it transitions from closed to open. To suppress this behavior, assign `focusOnOpen` to `true`. This should only be assigned when an appropriately accessible substitute behavior exists.
+By default, the popover will receive focus when it mounts. To suppress this behavior, assign `focusOnMount` to `false`. This should only be assigned when an appropriately accessible substitute behavior exists.
 
 - Type: `Boolean`
 - Required: No

diff --git a/components/popover/index.js b/components/popover/index.js
@@ -52,11 +52,10 @@ class Popover extends Component {
 	}
 
 	componentDidMount() {
-		if ( this.props.isOpen ) {
-			this.setOffset();
-			this.setForcedPositions();
-			this.toggleWindowEvents( true );
-		}
+		this.setOffset();
+		this.setForcedPositions();
+		this.toggleWindowEvents( true );
+		this.focus();
 	}
 
 	componentWillReceiveProps( nextProps ) {
@@ -69,21 +68,10 @@ class Popover extends Component {
 	}
 
 	componentDidUpdate( prevProps, prevState ) {
-		const { isOpen, position } = this.props;
-		const { isOpen: prevIsOpen, position: prevPosition } = prevProps;
-		if ( isOpen !== prevIsOpen ) {
-			this.toggleWindowEvents( isOpen );
-
-			if ( isOpen ) {
-				this.focus();
-			}
-		}
+		const { position } = this.props;
+		const { position: prevPosition } = prevProps;
 
-		if ( ! isOpen ) {
-			return;
-		}
-
-		if ( isOpen !== prevIsOpen || position !== prevPosition ) {
+		if ( position !== prevPosition ) {
 			this.setOffset();
 			this.setForcedPositions();
 		} else if ( ! isEqual( this.state, prevState ) ) {
@@ -105,8 +93,8 @@ class Popover extends Component {
 	}
 
 	focus() {
-		const { focusOnOpen = true } = this.props;
-		if ( ! focusOnOpen ) {
+		const { focusOnMount = true } = this.props;
+		if ( ! focusOnMount ) {
 			return;
 		}
 
@@ -129,7 +117,7 @@ class Popover extends Component {
 		this.rafHandle = window.requestAnimationFrame( this.setOffset );
 	}
 
-	getAnchorRect( ) {
+	getAnchorRect() {
 		const { anchor } = this.nodes;
 		if ( ! anchor || ! anchor.parentNode ) {
 			return;
@@ -251,7 +239,6 @@ class Popover extends Component {
 
 	render() {
 		const {
-			isOpen,
 			onClose,
 			children,
 			className,
@@ -261,18 +248,14 @@ class Popover extends Component {
 			/* eslint-disable no-unused-vars */
 			position,
 			range,
-			focusOnOpen,
+			focusOnMount,
 			getAnchorRect,
 			expandOnMobile,
 			/* eslint-enable no-unused-vars */
 			...contentProps
 		} = this.props;
 		const [ yAxis, xAxis ] = this.getPositions();
 
-		if ( ! isOpen ) {
-			return null;
-		}
-
 		const classes = classnames(
 			'components-popover',
 			className,
@@ -314,7 +297,7 @@ class Popover extends Component {
 
 		// Apply focus return behavior except when default focus on open
 		// behavior is disabled.
-		if ( false !== focusOnOpen ) {
+		if ( false !== focusOnMount ) {
 			content = <FocusManaged>{ content }</FocusManaged>;
 		}