From 30211a63bef327f759cf290156bb0bbaeda8af9e Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 12 Feb 2018 20:02:17 -0500 Subject: [PATCH 1/3] Components: Conditionally render popover --- components/autocomplete/index.js | 57 +++++++++++----------- components/autocomplete/test/index.js | 18 +++---- components/dropdown-menu/test/index.js | 4 +- components/dropdown/index.js | 25 +++++----- components/dropdown/test/index.js | 12 ++--- components/popover/README.md | 29 +++++------ components/popover/index.js | 41 +++++----------- components/popover/test/index.js | 67 ++++++++++---------------- components/tooltip/index.js | 19 ++++---- components/tooltip/test/index.js | 32 ++++++++---- 10 files changed, 141 insertions(+), 163 deletions(-) diff --git a/components/autocomplete/index.js b/components/autocomplete/index.js index 74ec27e69b6a92..13cb632ac404fd 100644 --- 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 } ) } - -
- { isExpanded && map( filteredOptions, ( option, index ) => ( - - ) ) } -
- +
+ { isExpanded && map( filteredOptions, ( option, index ) => ( + + ) ) } +
+ + ) } ); /* 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 index c5d780eedc8821..6abba94aeb5b1b 100644 --- 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 index bcfde6cf10fbd0..ea6cedfcc8e5d1 100644 --- 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 index 449e7afc3afe8a..9b97565ffbf594 100644 --- a/components/dropdown/index.js +++ b/components/dropdown/index.js @@ -80,18 +80,19 @@ class Dropdown extends Component { */ }
{ renderToggle( args ) } - - - { renderContent( args ) } - - + { isOpen && ( + + + { renderContent( args ) } + + + ) }
); diff --git a/components/dropdown/test/index.js b/components/dropdown/test/index.js index c2dca82617c44f..9f43daf1959fd9 100644 --- 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 index 53a0ef352d21fa..54577251f30bbd 100644 --- a/components/popover/README.md +++ b/components/popover/README.md @@ -14,18 +14,21 @@ function ToggleButton( { isVisible, toggleVisible } ) { return ( ); } ``` +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 index 93f2359c8bc98f..f10e52f2d60ab7 100644 --- 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,7 +248,7 @@ class Popover extends Component { /* eslint-disable no-unused-vars */ position, range, - focusOnOpen, + focusOnMount, getAnchorRect, expandOnMobile, /* eslint-enable no-unused-vars */ @@ -269,10 +256,6 @@ class Popover extends Component { } = 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 = { content }; } diff --git a/components/popover/test/index.js b/components/popover/test/index.js index fdf4f1e2e22389..8fa4b179a2ed7e 100644 --- a/components/popover/test/index.js +++ b/components/popover/test/index.js @@ -2,6 +2,7 @@ * External dependencies */ import { shallow, mount } from 'enzyme'; +import { noop } from 'lodash'; /** * Internal dependencies @@ -10,61 +11,52 @@ import Popover from '../'; describe( 'Popover', () => { describe( '#componentDidUpdate()', () => { - let wrapper, mocks; + let wrapper; beforeEach( () => { + jest.spyOn( Popover.prototype, 'toggleWindowEvents' ).mockImplementation( noop ); + jest.spyOn( Popover.prototype, 'setOffset' ).mockImplementation( noop ); + jest.spyOn( Popover.prototype, 'setForcedPositions' ).mockImplementation( noop ); + wrapper = shallow( , { lifecycleExperimental: true } ); + } ); - mocks = { - toggleWindowEvents: jest.fn(), - setOffset: jest.fn(), - setForcedPositions: jest.fn(), - }; - - Object.assign( wrapper.instance(), mocks ); + afterEach( () => { + jest.restoreAllMocks(); } ); it( 'should add window events', () => { - wrapper.setProps( { isOpen: true } ); - - expect( mocks.toggleWindowEvents ).toHaveBeenCalledWith( true ); - expect( mocks.setOffset ).toHaveBeenCalled(); - expect( mocks.setForcedPositions ).toHaveBeenCalled(); + expect( Popover.prototype.toggleWindowEvents ).toHaveBeenCalledWith( true ); + expect( Popover.prototype.setOffset ).toHaveBeenCalled(); + expect( Popover.prototype.setForcedPositions ).toHaveBeenCalled(); } ); it( 'should remove window events', () => { - wrapper.setProps( { isOpen: true } ); - jest.clearAllMocks(); - - wrapper.setProps( { isOpen: false } ); + wrapper.unmount(); - expect( mocks.toggleWindowEvents ).toHaveBeenCalledWith( false ); - expect( mocks.setOffset ).not.toHaveBeenCalled(); - expect( mocks.setForcedPositions ).not.toHaveBeenCalled(); + expect( Popover.prototype.toggleWindowEvents ).toHaveBeenCalledWith( false ); } ); it( 'should set offset and forced positions on changed position', () => { - wrapper.setProps( { isOpen: true } ); jest.clearAllMocks(); wrapper.setProps( { position: 'bottom right' } ); - expect( mocks.toggleWindowEvents ).not.toHaveBeenCalled(); - expect( mocks.setOffset ).toHaveBeenCalled(); - expect( mocks.setForcedPositions ).toHaveBeenCalled(); + expect( Popover.prototype.toggleWindowEvents ).not.toHaveBeenCalled(); + expect( Popover.prototype.setOffset ).toHaveBeenCalled(); + expect( Popover.prototype.setForcedPositions ).toHaveBeenCalled(); } ); it( 'should set offset on changed forced positions', () => { - wrapper.setProps( { isOpen: true } ); jest.clearAllMocks(); wrapper.setState( { forcedXAxis: 'left' } ); - expect( mocks.toggleWindowEvents ).not.toHaveBeenCalled(); - expect( mocks.setOffset ).toHaveBeenCalled(); - expect( mocks.setForcedPositions ).not.toHaveBeenCalled(); + expect( Popover.prototype.toggleWindowEvents ).not.toHaveBeenCalled(); + expect( Popover.prototype.setOffset ).toHaveBeenCalled(); + expect( Popover.prototype.setForcedPositions ).not.toHaveBeenCalled(); } ); it( 'should focus when opening', () => { @@ -72,7 +64,6 @@ describe( 'Popover', () => { // child, but in context of JSDOM the inputs are not visible and // are therefore skipped as tabbable, defaulting to popover. wrapper = mount( ); - wrapper.setProps( { isOpen: true } ); const content = wrapper.find( '.components-popover__content' ).getDOMNode(); @@ -82,8 +73,7 @@ describe( 'Popover', () => { it( 'should allow focus-on-open behavior to be disabled', () => { const activeElement = document.activeElement; - wrapper = mount( ); - wrapper.setProps( { isOpen: true } ); + wrapper = mount( ); expect( document.activeElement ).toBe( activeElement ); } ); @@ -282,20 +272,15 @@ describe( 'Popover', () => { } ); describe( '#render()', () => { - it( 'should render nothing if popover is not open', () => { - const wrapper = shallow( ); - - expect( wrapper.type() ).toBeNull(); - } ); - - it( 'should render content if popover is open', () => { - const wrapper = shallow( Hello, { disableLifecycleMethods: true } ); + it( 'should render content', () => { + const wrapper = shallow( Hello, { disableLifecycleMethods: true } ); - expect( wrapper.type() ).not.toBeNull(); + expect( wrapper.type() ).toBe( 'span' ); + expect( wrapper.find( '.components-popover__content' ).prop( 'children' ) ).toBe( 'Hello' ); } ); it( 'should pass additional to portaled element', () => { - const wrapper = shallow( Hello, { disableLifecycleMethods: true } ); + const wrapper = shallow( Hello, { disableLifecycleMethods: true } ); expect( wrapper.find( '.components-popover' ).prop( 'role' ) ).toBe( 'tooltip' ); } ); diff --git a/components/tooltip/index.js b/components/tooltip/index.js index 3b1a6dc9f4b23c..a4a444d6ca1ca1 100644 --- a/components/tooltip/index.js +++ b/components/tooltip/index.js @@ -171,15 +171,16 @@ class Tooltip extends Component { onBlur: this.createToggleIsOver( 'onBlur' ), children: concatChildren( child.props.children, - , + isOver && ( + + ), ), } ); } diff --git a/components/tooltip/test/index.js b/components/tooltip/test/index.js index 64336b2ce0d496..57e9b1a674eee4 100644 --- a/components/tooltip/test/index.js +++ b/components/tooltip/test/index.js @@ -19,21 +19,35 @@ describe( 'Tooltip', () => { expect( wrapper.children() ).toHaveLength( 2 ); } ); - it( 'should render children with additional popover', () => { + it( 'should render children', () => { const wrapper = shallow( ); + const button = wrapper.find( 'button' ); + expect( wrapper.type() ).toBe( 'button' ); + expect( button.children() ).toHaveLength( 1 ); + expect( button.childAt( 0 ).text() ).toBe( 'Hover Me!' ); + } ); + + it( 'should render children with additional popover when over', () => { + const wrapper = shallow( + + + + ); + + wrapper.setState( { isOver: true } ); + const button = wrapper.find( 'button' ); const popover = wrapper.find( 'Popover' ); expect( wrapper.type() ).toBe( 'button' ); expect( button.children() ).toHaveLength( 2 ); expect( button.childAt( 0 ).text() ).toBe( 'Hover Me!' ); expect( button.childAt( 1 ).name() ).toBe( 'Popover' ); - expect( popover.prop( 'isOpen' ) ).toBe( false ); - expect( popover.prop( 'focusOnOpen' ) ).toBe( false ); + expect( popover.prop( 'focusOnMount' ) ).toBe( false ); expect( popover.prop( 'position' ) ).toBe( 'bottom right' ); expect( popover.children().text() ).toBe( 'Help text' ); } ); @@ -58,11 +72,11 @@ describe( 'Tooltip', () => { const popover = wrapper.find( 'Popover' ); expect( originalFocus ).toHaveBeenCalledWith( event ); expect( wrapper.state( 'isOver' ) ).toBe( true ); - expect( popover.prop( 'isOpen' ) ).toBe( true ); + expect( popover ).toHaveLength( 1 ); } ); it( 'should show popover on delayed mouseenter', () => { - const expectPopoverOpened = ( wrapper, opened ) => expect( wrapper.find( 'Popover' ) ).toHaveProp( 'isOpen', opened ); + const expectPopoverVisible = ( wrapper, visible ) => expect( wrapper.find( 'Popover' ) ).toHaveLength( visible ? 1 : 0 ); // Mount: Issues with using `setState` asynchronously with shallow- // rendered components: https://github.com/airbnb/enzyme/issues/450 @@ -88,12 +102,12 @@ describe( 'Tooltip', () => { expect( originalMouseEnter ).toHaveBeenCalled(); expect( wrapper.state( 'isOver' ) ).toBe( false ); - expectPopoverOpened( wrapper, false ); + expectPopoverVisible( wrapper, false ); wrapper.instance().delayedSetIsOver.flush(); wrapper.update(); expect( wrapper.state( 'isOver' ) ).toBe( true ); - expectPopoverOpened( wrapper, true ); + expectPopoverVisible( wrapper, true ); } ); it( 'should ignore mouseenter on disabled elements', () => { @@ -124,7 +138,7 @@ describe( 'Tooltip', () => { const popover = wrapper.find( 'Popover' ); wrapper.instance().delayedSetIsOver.flush(); expect( wrapper.state( 'isOver' ) ).toBe( false ); - expect( popover.prop( 'isOpen' ) ).toBe( false ); + expect( popover ).toHaveLength( 0 ); } ); it( 'should cancel pending setIsOver on mouseleave', () => { @@ -150,7 +164,7 @@ describe( 'Tooltip', () => { const popover = wrapper.find( 'Popover' ); expect( wrapper.state( 'isOver' ) ).toBe( false ); - expect( popover.prop( 'isOpen' ) ).toBe( false ); + expect( popover ).toHaveLength( 0 ); } ); } ); } ); From d243c9de56d97138552ff71abd026c6d03cf4ead Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 12 Feb 2018 20:03:01 -0500 Subject: [PATCH 2/3] Components: Remove redundant Dropdown FocusManaged Already applied by Popover in all cases except when focusOnMount is false --- components/dropdown/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/components/dropdown/index.js b/components/dropdown/index.js index 9b97565ffbf594..ff1a68f2e00f42 100644 --- a/components/dropdown/index.js +++ b/components/dropdown/index.js @@ -88,9 +88,7 @@ class Dropdown extends Component { onClickOutside={ this.clickOutside } expandOnMobile={ expandOnMobile } > - - { renderContent( args ) } - + { renderContent( args ) } ) } From 1b27293c49a6accf690b44a7f14cf862acc5f4d4 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 12 Feb 2018 20:33:27 -0500 Subject: [PATCH 3/3] Components: Remove unused FocusManaged --- components/dropdown/index.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/components/dropdown/index.js b/components/dropdown/index.js index ff1a68f2e00f42..6f46edf58ba742 100644 --- 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 );