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.

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

Title: Unselect title by blur event (B) #2974

Closed
wants to merge 14 commits into from
Closed
40 changes: 34 additions & 6 deletions components/clipboard-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,16 @@ import { findDOMNode, Component } from '@wordpress/element';
*/
import { Button } from '../';

class ClipboardButton extends Component {
// This creates a container to put the textarea in which isn't removed by react
// If react removes the textarea first, then the clipboard fails when trying to remove it
class ClipboardContainer extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a separate component, or can we assign the button of ClipboardButton as the container?

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 13, 2017

Choose a reason for hiding this comment

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

It needs to have shouldComponentUpdate return false so that the container with the clipboard textarea isn't removed from the DOM. The problem is if it gets redrawn, then the textarea that is inside gets removed, and the clipboard library throws an error when trying to remove it. We just need a component that can get things added to it, and isn't going to get recreated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it as the button originally, and kept getting an error in the clipboard library when it tried to remove the textarea, because react had already removed it because the button had been redrawn. You would get this error if you tried clicking on the button after you had already clicked on it. Therefore, the idea was to make a component that could have anything inside it, and react wouldn't keep trying to remove it. It's a limitation of using a third party library which appends components into the DOM. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't supply container, then it defaults to body, which is fine normally, but it will fire a blur which will trigger focus outside. That's why I'm supplying container ... to keep the focus inside the post title block.

componentDidMount() {
const { text, onCopy = noop } = this.props;
const button = findDOMNode( this.button );
this.clipboard = new Clipboard( button, {
const { text, buttonNode, onCopy = noop } = this.props;
this.clipboard = new Clipboard( buttonNode, {
text: () => text,
// If we put the textarea in a specific container, then the focus stays
// within this container (for use in whenFocusOutside)
container: this.container,
} );
this.clipboard.on( 'success', onCopy );
}
Expand All @@ -30,16 +34,40 @@ class ClipboardButton extends Component {
delete this.clipboard;
}

shouldComponentUpdate() {
return false;
}

render() {
return <div ref={ ref => this.container = ref } />;
}
}

class ClipboardButton extends Component {
constructor() {
super( ...arguments );
this.bindButton = this.bindButton.bind( this );
}

bindButton( ref ) {
if ( ref ) {
this.button = ref;
// Need to pass the button node down to use as the trigger
// The first rendering of ClipboardContainer it's null
this.forceUpdate();
}
}
render() {
const { className, children } = this.props;
const { className, children, onCopy, text } = this.props;
const classes = classnames( 'components-clipboard-button', className );

return (
<Button
ref={ ref => this.button = ref }
ref={ this.bindButton }
className={ classes }
>
{ children }
{ this.button && <ClipboardContainer buttonNode={ findDOMNode( this.button ) } onCopy={ onCopy } text={ text } /> }
</Button>
);
}
Expand Down
69 changes: 69 additions & 0 deletions components/higher-order/with-focus-outside/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/**
* External dependencies
*/
import hoistNonReactStatic from 'hoist-non-react-statics';

/**
* WordPress dependencies
*/
import { Component, findDOMNode } from '@wordpress/element';

/* Heavily based on react-click-outside (https://github.com/kentor/react-click-outside/blob/master/index.js),
* this Higher Order Component wraps a component and fires any handleFocusOutside listeners it might have
* if a focus is detected ouside that component
*
* @param {WPElement} OriginalComponent the original component
*
* @return {Component} Component with focus outside detection
*/

function withFocusOutside( OriginalComponent ) {
const componentName = OriginalComponent.displayName || OriginalComponent.name;

class EnhancedComponent extends Component {
constructor() {
super( ...arguments );
this.onFocusOutside = this.onFocusOutside.bind( this );
this.bindRef = this.bindRef.bind( this );
}

componentDidMount() {
document.addEventListener( 'focusin', this.onFocusOutside, true );
}

componentWillUnmount() {
document.removeEventListener( 'focusin', this.onFocusOutside, true );
}

onFocusOutside( e ) {
const domNode = this.__domNode;
if (
( ! domNode || ! domNode.contains( e.target ) ) &&
typeof this.__wrappedInstance.handleFocusOutside === 'function'
) {
this.__wrappedInstance.handleFocusOutside( e );
}
}

bindRef( ref ) {
this.__wrappedInstance = ref;
// eslint-disable-next-line react/no-find-dom-node
this.__domNode = findDOMNode( ref );
}

render() {
return (
<OriginalComponent
{ ...this.props }
ref={ this.bindRef }
/>
);
}
}

EnhancedComponent.displayName = `FocusOutside(${ componentName })`;

return hoistNonReactStatic( EnhancedComponent, OriginalComponent );
}

export default withFocusOutside;
38 changes: 38 additions & 0 deletions components/higher-order/with-focus-outside/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';
import { Component } from '../../../../element';

/**
* Internal dependencies
*/
import withFocusOutside from '../';

class Test extends Component {
render() {
return (
<div className="test">Testing</div>
);
}
}

describe( 'withFocusOutside()', () => {
const Composite = withFocusOutside( Test );

it( 'should render a basic Test component inside the HOC', () => {
const renderedComposite = shallow( <Composite /> );
const wrappedElement = renderedComposite.find( Test );
const wrappedElementShallow = wrappedElement.shallow();
expect( wrappedElementShallow.hasClass( 'test' ) ).toBe( true );
expect( wrappedElementShallow.type() ).toBe( 'div' );
expect( wrappedElementShallow.text() ).toBe( 'Testing' );
} );

it( 'should pass additional props through to the wrapped element', () => {
const renderedComposite = shallow( <Composite test="test" /> );
const wrappedElement = renderedComposite.find( Test );
// Ensure that the wrapped Test element has the appropriate props.
expect( wrappedElement.props().test ).toBe( 'test' );
} );
} );
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ export { default as Tooltip } from './tooltip';
// Higher-Order Components
export { default as withAPIData } from './higher-order/with-api-data';
export { default as withFocusReturn } from './higher-order/with-focus-return';
export { default as withFocusOutside } from './higher-order/with-focus-outside';
export { default as withInstanceId } from './higher-order/with-instance-id';
export { default as withSpokenMessages } from './higher-order/with-spoken-messages';
2 changes: 2 additions & 0 deletions editor/post-permalink/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class PostPermalink extends Component {
showCopyConfirmation: false,
} );
}, 4000 );

this.props.onLinkCopied();
}

render() {
Expand Down
35 changes: 27 additions & 8 deletions editor/post-title/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { connect } from 'react-redux';
import Textarea from 'react-autosize-textarea';
import clickOutside from 'react-click-outside';
import classnames from 'classnames';

/**
Expand All @@ -12,6 +11,7 @@ import classnames from 'classnames';
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { keycodes } from '@wordpress/utils';
import { withFocusOutside } from '@wordpress/components';

/**
* Internal dependencies
Expand All @@ -35,8 +35,13 @@ class PostTitle extends Component {
this.onSelect = this.onSelect.bind( this );
this.onUnselect = this.onUnselect.bind( this );
this.onSelectionChange = this.onSelectionChange.bind( this );
this.onContainerFocus = this.onContainerFocus.bind( this );
this.setFocused = this.setFocused.bind( this );
this.focusText = this.focusText.bind( this );
this.handleFocusOutside = this.handleFocusOutside.bind( this );
this.state = {
isSelected: false,
hasFocusWithin: false,
};
}

Expand All @@ -62,6 +67,10 @@ class PostTitle extends Component {
}
}

focusText() {
this.textareaContainer.textarea.focus();
}

onChange( event ) {
const newTitle = event.target.value.replace( REGEXP_NEWLINES, ' ' );
this.props.onUpdate( newTitle );
Expand All @@ -76,8 +85,16 @@ class PostTitle extends Component {
this.setState( { isSelected: false } );
}

handleClickOutside() {
this.setState( { isSelected: false } );
setFocused( focused ) {
this.setState( { hasFocusWithin: focused } );
}

handleFocusOutside() {
this.setFocused( false );
}

onContainerFocus() {
this.setFocused( true );
}

onKeyDown( event ) {
Expand All @@ -88,12 +105,14 @@ class PostTitle extends Component {

render() {
const { title } = this.props;
const { isSelected } = this.state;
const className = classnames( 'editor-post-title', { 'is-selected': isSelected } );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe isSelected should be something like isTyping?

Copy link
Member

Choose a reason for hiding this comment

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

The naming had confused me as well when I first encountered it. I think isTyping would be an improvement, yes.

const { isSelected, hasFocusWithin } = this.state;
const className = classnames( 'editor-post-title', { 'is-selected': isSelected && hasFocusWithin } );

return (
<div className={ className }>
{ isSelected && <PostPermalink /> }
<div
className={ className }
onFocus={ this.onContainerFocus }>
{ isSelected && hasFocusWithin && <PostPermalink onLinkCopied={ this.focusText } /> }
<h1>
<Textarea
ref={ this.bindTextarea }
Expand Down Expand Up @@ -126,4 +145,4 @@ export default connect(
},
};
}
)( clickOutside( PostTitle ) );
)( withFocusOutside( PostTitle ) );