Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: Port Popover to use withViewportMatch #5316

Closed
wants to merge 3 commits into from
Closed

Components: Port Popover to use withViewportMatch #5316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33d8eb5
Components: Port Popovert to use withViewportMatch
aduth Feb 28, 2018
507132b
Components: Expose wrapped Popover for tests
aduth Feb 28, 2018
56cbd7a
Unit Tests: Find Popover component by reference
aduth Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions components/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { isEqual, noop } from 'lodash';
*/
import { Component } from '@wordpress/element';
import { focus, keycodes } from '@wordpress/utils';
import { withViewportMatch } from '@wordpress/viewport';

/**
* Internal dependencies
Expand All @@ -29,7 +30,6 @@ const { ESCAPE } = keycodes;
* @type {String}
*/
const SLOT_NAME = 'Popover';
const isMobile = () => window.innerWidth < 782;

class Popover extends Component {
constructor() {
Expand All @@ -47,7 +47,6 @@ class Popover extends Component {
this.state = {
forcedYAxis: null,
forcedXAxis: null,
isMobile: false,
};
}

Expand Down Expand Up @@ -140,28 +139,17 @@ class Popover extends Component {
}

setOffset() {
const { getAnchorRect = this.getAnchorRect, expandOnMobile = false } = this.props;
const { getAnchorRect = this.getAnchorRect, isMobile, expandOnMobile = false } = this.props;
const { popover } = this.nodes;

if ( isMobile() && expandOnMobile ) {
if ( isMobile && expandOnMobile ) {
popover.style.left = 0;
popover.style.top = 0;
popover.style.right = 0;
popover.style.bottom = 0;
if ( ! this.state.isMobile ) {
this.setState( {
isMobile: true,
} );
}
return;
}

if ( this.state.isMobile ) {
this.setState( {
isMobile: false,
} );
}

const [ yAxis, xAxis ] = this.getPositions();
const isTop = 'top' === yAxis;
const isLeft = 'left' === xAxis;
Expand Down Expand Up @@ -250,6 +238,7 @@ class Popover extends Component {
range,
focusOnMount,
getAnchorRect,
isMobile,
expandOnMobile,
/* eslint-enable no-unused-vars */
...contentProps
Expand All @@ -262,7 +251,7 @@ class Popover extends Component {
'is-' + yAxis,
'is-' + xAxis,
{
'is-mobile': this.state.isMobile,
'is-mobile': isMobile,
}
);

Expand All @@ -278,7 +267,7 @@ class Popover extends Component {
{ ...contentProps }
onKeyDown={ this.maybeClose }
>
{ this.state.isMobile && (
{ isMobile && (
<div className="components-popover__header">
<IconButton className="components-popover__close" icon="no-alt" onClick={ onClose } />
</div>
Expand Down Expand Up @@ -316,6 +305,17 @@ Popover.contextTypes = {
getSlot: noop,
};

const _Popover = Popover;

Popover = withViewportMatch( {
isMobile: '< small',
} )( Popover );

// TODO: This is a useful pattern for retrieving the unmodified component from
// within unit tests, and might be something to consider building into a common
// `createHigherOrderComponent` helper function (along with `displayName`).
Popover.WrappedComponent = _Popover;

Popover.Slot = () => <Slot bubblesVirtually name={ SLOT_NAME } />;

export default Popover;
4 changes: 3 additions & 1 deletion components/popover/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import { noop } from 'lodash';
/**
* Internal dependencies
*/
import Popover from '../';
import EnhancedPopover from '../';

const Popover = EnhancedPopover.WrappedComponent;

describe( 'Popover', () => {
describe( '#componentDidUpdate()', () => {
Expand Down
13 changes: 7 additions & 6 deletions components/tooltip/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { shallow, mount } from 'enzyme';
* Internal dependencies
*/
import Tooltip from '../';
import Popover from '../../popover';

describe( 'Tooltip', () => {
describe( '#render()', () => {
Expand Down Expand Up @@ -42,11 +43,11 @@ describe( 'Tooltip', () => {
wrapper.setState( { isOver: true } );

const button = wrapper.find( 'button' );
const popover = wrapper.find( 'Popover' );
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( button.childAt( 1 ).type() ).toBe( Popover );
expect( popover.prop( 'focusOnMount' ) ).toBe( false );
expect( popover.prop( 'position' ) ).toBe( 'bottom right' );
expect( popover.children().text() ).toBe( 'Help text' );
Expand All @@ -69,14 +70,14 @@ describe( 'Tooltip', () => {
const button = wrapper.find( 'button' );
button.simulate( 'focus', event );

const popover = wrapper.find( 'Popover' );
const popover = wrapper.find( Popover );
expect( originalFocus ).toHaveBeenCalledWith( event );
expect( wrapper.state( 'isOver' ) ).toBe( true );
expect( popover ).toHaveLength( 1 );
} );

it( 'should show popover on delayed mouseenter', () => {
const expectPopoverVisible = ( wrapper, visible ) => expect( wrapper.find( 'Popover' ) ).toHaveLength( visible ? 1 : 0 );
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
Expand Down Expand Up @@ -135,7 +136,7 @@ describe( 'Tooltip', () => {

expect( originalMouseEnter ).toHaveBeenCalled();

const popover = wrapper.find( 'Popover' );
const popover = wrapper.find( Popover );
wrapper.instance().delayedSetIsOver.flush();
expect( wrapper.state( 'isOver' ) ).toBe( false );
expect( popover ).toHaveLength( 0 );
Expand All @@ -162,7 +163,7 @@ describe( 'Tooltip', () => {

wrapper.instance().delayedSetIsOver.flush();

const popover = wrapper.find( 'Popover' );
const popover = wrapper.find( Popover );
expect( wrapper.state( 'isOver' ) ).toBe( false );
expect( popover ).toHaveLength( 0 );
} );
Expand Down
4 changes: 2 additions & 2 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ function gutenberg_register_scripts_and_styles() {
wp_register_script(
'wp-components',
gutenberg_url( 'components/build/index.js' ),
array( 'wp-element', 'wp-i18n', 'wp-utils', 'wp-hooks', 'wp-api-request', 'moment' ),
array( 'wp-element', 'wp-i18n', 'wp-utils', 'wp-hooks', 'wp-api-request', 'wp-viewport', 'moment' ),
filemtime( gutenberg_dir_path() . 'components/build/index.js' )
);
wp_register_script(
Expand All @@ -161,7 +161,7 @@ function gutenberg_register_scripts_and_styles() {
wp_register_script(
'wp-viewport',
gutenberg_url( 'viewport/build/index.js' ),
array( 'wp-element', 'wp-data', 'wp-components' ),
array( 'wp-element', 'wp-data' ),
filemtime( gutenberg_dir_path() . 'viewport/build/index.js' )
);
// Loading the old editor and its config to ensure the classic block works as expected.
Expand Down
20 changes: 19 additions & 1 deletion viewport/if-viewport-matches.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,25 @@
* WordPress dependencies
*/
import { compose, getWrapperDisplayName } from '@wordpress/element';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer an issue since we extracted compose package.

import { ifCondition } from '@wordpress/components';

// TEMPORARY: A circular dependency exists between `@wordpress/components`
// and `@wordpress/viewport` (Popover -> withViewportMatch, ifViewportMatches
// -> ifCondition).

// import { ifCondition } from '@wordpress/components';
const ifCondition = ( predicate ) => ( WrappedComponent ) => {
const EnhancedComponent = ( props ) => {
if ( ! predicate( props ) ) {
return null;
}

return <WrappedComponent { ...props } />;
};

EnhancedComponent.displayName = getWrapperDisplayName( WrappedComponent, 'ifCondition' );

return EnhancedComponent;
};

/**
* Internal dependencies
Expand Down