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

Popover: Refactor popover to clarify computations #6799

Merged
merged 11 commits into from
May 23, 2018
Merged

Popover: Refactor popover to clarify computations #6799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3a34c44
Popover: Refactor popover to clarify computations and support setting…
youknowriad May 17, 2018
66b3444
Fix popover z-index
youknowriad May 17, 2018
05e8db6
Adding unit tests
youknowriad May 17, 2018
63cf64c
Fix inserter popover
youknowriad May 17, 2018
a445099
Remove useless check
youknowriad May 17, 2018
5884da6
Fix e2e tests
youknowriad May 21, 2018
2e1f8d3
Fix unit tests
youknowriad May 22, 2018
8788478
All the popovers should show up above the UI
youknowriad May 22, 2018
9ad4297
Leave more space for the arrow
youknowriad May 22, 2018
a96d4cd
Set timeout moved to the focus handler
youknowriad May 23, 2018
3e1d6be
Split the popover utility to smaller functions and test the mobile case
youknowriad May 23, 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
10 changes: 2 additions & 8 deletions components/autocomplete/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,18 +535,12 @@ export class Autocomplete extends Component {
event.stopPropagation();
}

getWordRect( { isLeft, isRight } ) {
getWordRect() {
const { range } = this.state;
if ( ! range ) {
return;
}
if ( isLeft ) {
const rects = range.getClientRects();
return rects[ 0 ];
} else if ( isRight ) {
const rects = range.getClientRects();
return rects[ rects.length - 1 ];
}

return range.getBoundingClientRect();
}

Expand Down
248 changes: 141 additions & 107 deletions components/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { isEqual, noop } from 'lodash';
import { noop } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -31,7 +31,7 @@ const { ESCAPE } = keycodes;
* @type {String}
*/
const SLOT_NAME = 'Popover';
const isMobile = () => window.innerWidth < 782;
const isMobileViewport = () => window.innerWidth < 782;

class Popover extends Component {
constructor() {
Expand All @@ -40,45 +40,34 @@ class Popover extends Component {
this.focus = this.focus.bind( this );
this.bindNode = this.bindNode.bind( this );
this.getAnchorRect = this.getAnchorRect.bind( this );
this.setOffset = this.setOffset.bind( this );
this.throttledSetOffset = this.throttledSetOffset.bind( this );
this.computePopoverPosition = this.computePopoverPosition.bind( this );
this.throttledComputePopoverPosition = this.throttledComputePopoverPosition.bind( this );
this.maybeClose = this.maybeClose.bind( this );

this.nodes = {};

this.state = {
forcedYAxis: null,
forcedXAxis: null,
popoverLeft: null,
popoverTop: null,
yAxis: 'top',
xAxis: 'center',
contentHeight: null,
contentWidth: null,
isMobile: false,
popoverSize: null,
};
}

componentDidMount() {
this.setOffset();
this.setForcedPositions();
this.toggleWindowEvents( true );
const popoverSize = this.updatePopoverSize();
this.computePopoverPosition( popoverSize );
this.focus();
this.toggleWindowEvents( true );
}

componentWillReceiveProps( nextProps ) {
if ( this.props.position !== nextProps.position ) {
this.setState( {
forcedYAxis: null,
forcedXAxis: null,
} );
}
}

componentDidUpdate( prevProps, prevState ) {
const { position } = this.props;
const { position: prevPosition } = prevProps;

if ( position !== prevPosition ) {
this.setOffset();
this.setForcedPositions();
} else if ( ! isEqual( this.state, prevState ) ) {
// Need to update offset if forced positioning applied
this.setOffset();
componentDidUpdate( prevProps ) {
if ( prevProps.position !== this.props.position ) {
this.computePopoverPosition();
}
}

Expand All @@ -90,8 +79,15 @@ class Popover extends Component {
const handler = isListening ? 'addEventListener' : 'removeEventListener';

window.cancelAnimationFrame( this.rafHandle );
window[ handler ]( 'resize', this.throttledSetOffset );
window[ handler ]( 'scroll', this.throttledSetOffset, true );
window[ handler ]( 'resize', this.throttledComputePopoverPosition );
window[ handler ]( 'scroll', this.throttledComputePopoverPosition, true );
}

throttledComputePopoverPosition( event ) {
if ( event.type === 'scroll' && this.nodes.content.contains( event.target ) ) {
return;
}
this.rafHandle = window.requestAnimationFrame( () => this.computePopoverPosition() );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: maybe we should save the id returned by requestAnimationFrame and invoke window.cancelAnimationFrame() when unMounting if we have a valid id to cancel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already the case I believe

}

focus() {
Expand All @@ -115,10 +111,6 @@ class Popover extends Component {
}
}

throttledSetOffset() {
this.rafHandle = window.requestAnimationFrame( this.setOffset );
}

getAnchorRect() {
const { anchor } = this.nodes;
if ( ! anchor || ! anchor.parentNode ) {
Expand All @@ -141,83 +133,105 @@ class Popover extends Component {
};
}

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

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,
} );
updatePopoverSize() {
const { content } = this.nodes;
const rect = content.getBoundingClientRect();
if (
! this.state.popoverSize ||
rect.width !== this.state.popoverSize.width ||
rect.height !== this.state.popoverSize.height
) {
const popoverSize = {
height: rect.height,
width: rect.width,
};
this.setState( { popoverSize } );
return popoverSize;
}
return this.state.popoverSize;
}

const [ yAxis, xAxis ] = this.getPositions();
const isTop = 'top' === yAxis;
const isLeft = 'left' === xAxis;
const isRight = 'right' === xAxis;

const rect = getAnchorRect( { isTop, isLeft, isRight } );
if ( ! rect ) {
return;
}
computePopoverPosition( popoverSize ) {
const { width, height } = popoverSize || this.state.popoverSize;
const { getAnchorRect = this.getAnchorRect, position = 'top', expandOnMobile } = this.props;
const [ yAxis, xAxis = 'center' ] = position.split( ' ' );

popover.style.bottom = 'auto';
popover.style.right = 'auto';
const rect = getAnchorRect();
const popoverLeft = Math.round( rect.left + ( rect.width / 2 ) );

// Set popover at parent node center
popover.style.left = Math.round( rect.left + ( rect.width / 2 ) ) + 'px';
// y axis aligment choices
const topAlignment = {
popoverTop: rect.top,
contentHeight: rect.top - height > 0 ? height : rect.top,
};
const bottomAlignment = {
popoverTop: rect.bottom,
contentHeight: rect.bottom + height > window.innerHeight ? window.innerHeight - rect.bottom : height,
};

// Set at top or bottom of parent node based on popover position
popover.style.top = rect[ yAxis ] + 'px';
}
// x axis alignment choices
const centerAlignment = {
contentWidth: (
( popoverLeft - ( width / 2 ) > 0 ? ( width / 2 ) : popoverLeft ) +
( popoverLeft + ( width / 2 ) > window.innerWidth ? window.innerWidth - popoverLeft : ( width / 2 ) )
),
};
const leftAlignment = {
contentWidth: popoverLeft - width > 0 ? width : popoverLeft,
};
const rightAlignment = {
contentWidth: popoverLeft + width > window.innerWidth ? window.innerWidth - popoverLeft : width,
};

setForcedPositions() {
const anchor = this.getAnchorRect();
const rect = this.nodes.content.getBoundingClientRect();

// Check exceeding top or bottom of viewport and switch direction if the space is begger
if ( rect.top < 0 || rect.bottom > window.innerHeight ) {
const overflowBottom = window.innerHeight - ( anchor.bottom + rect.height );
const overflowTop = anchor.top - rect.height;
const direction = overflowTop < overflowBottom ? 'bottom' : 'top';
if ( direction !== this.state.forcedYAxis ) {
this.setState( { forcedYAxis: direction } );
}
// Choosing the y axis
let chosenYAxis;
let contentHeight = null;
if ( yAxis === 'top' && topAlignment.contentHeight === height ) {
chosenYAxis = 'top';
} else if ( yAxis === 'bottom' && bottomAlignment.contentHeight === height ) {
chosenYAxis = 'bottom';
} else {
chosenYAxis = topAlignment.contentHeight > bottomAlignment.contentHeight ? 'top' : 'bottom';
contentHeight = chosenYAxis === 'top' ? topAlignment.contentHeight : bottomAlignment.contentHeight;
}

// Check exceeding left or right of viewport and switch direction if the space is begger
if ( rect.left < 0 || rect.right > window.innerWidth ) {
const overflowLeft = anchor.left - rect.width;
const overflowRight = window.innerWidth - ( anchor.right + rect.width );
const direction = overflowLeft < overflowRight ? 'right' : 'left';
if ( direction !== this.state.forcedXAxis ) {
this.setState( { forcedXAxis: direction } );
}
// Choosing the x axis
let chosenXAxis;
let contentWidth = null;
if ( xAxis === 'center' && centerAlignment.contentWidth === width ) {
chosenXAxis = 'center';
} else if ( xAxis === 'left' && leftAlignment.contentWidth === width ) {
chosenXAxis = 'left';
} else if ( xAxis === 'right' && rightAlignment.contentWidth === width ) {
chosenXAxis = 'right';
} else {
chosenXAxis = leftAlignment.contentWidth > rightAlignment.contentWidth ? 'left' : 'right';
contentWidth = chosenXAxis === 'left' ? leftAlignment.contentWidth : rightAlignment.contentWidth;
}
}

getPositions() {
const { position = 'top' } = this.props;
const [ yAxis, xAxis = 'center' ] = position.split( ' ' );
const { forcedYAxis, forcedXAxis } = this.state;
const popoverTop = chosenYAxis === 'top' ? topAlignment.popoverTop : bottomAlignment.popoverTop;
const newPopoverPosition = {
isMobile: isMobileViewport() && expandOnMobile,
yAxis: chosenYAxis,
xAxis: chosenXAxis,
popoverTop,
popoverLeft,
contentHeight,
contentWidth,
};

return [
forcedYAxis || yAxis,
forcedXAxis || xAxis,
];
if (
! this.state.popoverLeft ||
Copy link
Member

Choose a reason for hiding this comment

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

I'm not understanding why we have the condition ! this.state.popoverLeftshouldn't this case be covered by this.state.popoverLeft !== popoverLeft? I think I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a leftover :)

this.state.yAxis !== chosenYAxis ||
this.state.xAxis !== chosenXAxis ||
this.state.popoverLeft !== popoverLeft ||
this.state.popoverTop !== popoverTop ||
this.state.contentHeight !== contentHeight ||
this.state.contentWidth !== contentWidth ||
this.state.isMobile !== newPopoverPosition.isMobile
) {
this.setState( newPopoverPosition );
}
}

maybeClose( event ) {
Expand Down Expand Up @@ -257,15 +271,24 @@ class Popover extends Component {
/* eslint-enable no-unused-vars */
...contentProps
} = this.props;
const [ yAxis, xAxis ] = this.getPositions();
const {
popoverLeft,
popoverTop,
yAxis,
xAxis,
contentHeight,
contentWidth,
popoverSize,
isMobile,
} = this.state;

const classes = classnames(
'components-popover',
className,
'is-' + yAxis,
'is-' + xAxis,
{
'is-mobile': this.state.isMobile,
'is-mobile': isMobile,
}
);

Expand All @@ -278,10 +301,15 @@ class Popover extends Component {
<div
ref={ this.bindNode( 'popover' ) }
className={ classes }
style={ {
top: ! isMobile && popoverTop ? popoverTop + 'px' : undefined,
left: ! isMobile && popoverLeft ? popoverLeft + 'px' : undefined,
visibility: popoverSize ? undefined : 'hidden',
} }
{ ...contentProps }
onKeyDown={ this.maybeClose }
>
{ this.state.isMobile && (
{ isMobile && (
<div className="components-popover__header">
<span className="components-popover__header-title">
{ headerTitle }
Expand All @@ -292,6 +320,10 @@ class Popover extends Component {
<div
ref={ this.bindNode( 'content' ) }
className="components-popover__content"
style={ {
maxHeight: ! isMobile && contentHeight ? contentHeight + 'px' : undefined,
maxWidth: ! isMobile && contentWidth ? contentWidth + 'px' : undefined,
} }
tabIndex="-1"
>
{ children }
Expand All @@ -316,15 +348,17 @@ class Popover extends Component {

return <span ref={ this.bindNode( 'anchor' ) }>
{ content }
{ this.state.isMobile && expandOnMobile && <ScrollLock /> }
{ isMobile && expandOnMobile && <ScrollLock /> }
</span>;
}
}

Popover.contextTypes = {
const PopoverContainer = Popover;

PopoverContainer.contextTypes = {
getSlot: noop,
};

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

export default Popover;
export default PopoverContainer;
8 changes: 8 additions & 0 deletions components/popover/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
z-index: z-index( ".components-popover" );
left: 50%;

&.is-mobile {
top: 0;
left: 0;
right: 0;
bottom: 0;
}

&:not(.is-mobile) {
margin-left: 2px;

Expand Down Expand Up @@ -82,6 +89,7 @@
position: absolute;
min-width: 240px;
height: auto;
overflow-y: auto;
}

.components-popover:not(.is-mobile).is-top & {
Expand Down
Loading