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
72 changes: 72 additions & 0 deletions components/higher-order/with-focus-outside/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/**
* 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 );
}

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( wrappedRefCallback, ref ) {
this.__wrappedInstance = ref;
// eslint-disable-next-line react/no-find-dom-node
this.__domNode = findDOMNode( ref );
if ( wrappedRefCallback ) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of the wrappedRefCallback? Is this just inherited from click outside? Probably best to avoid introducing features unless we foresee needing them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, based on the click outside. I can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this extra wrapping.

wrappedRefCallback( ref );
}
}

render() {
const { wrappedRef, ...rest } = this.props;
return (
<OriginalComponent
{ ...rest }
ref={ this.bindRef.bind( this, wrappedRef ) }
/>
);
}
}

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
38 changes: 30 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,19 @@ class PostTitle extends Component {
this.setState( { isSelected: false } );
}

handleClickOutside() {
this.setState( { isSelected: false } );
setFocused( focused ) {
this.setState( { hasFocusWithin: focused } );
if ( focused ) {
this.props.clearSelectedBlock();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Ideally the title focus handler shouldn't need to deal with block selection (i.e. it should be block focus leave events clearing its selected state).

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.

When I was testing it before, that wasn't working. But that was an earlier version. I'm happy to remove it and try and trust the blur of the other blocks.

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.

Removed this as it was not required. I think it was from an earlier version where I wasn't keeping isSelected separate.

}
}

handleFocusOutside( ) {
Copy link
Member

Choose a reason for hiding this comment

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

Styling note: There should be no spaces within the parentheses:

No filler spaces in empty constructs (e.g., {}, [], fn()).

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing

I'll plan to look and see if we can enforce this by ESLint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

this.setFocused( false );
}

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

onKeyDown( event ) {
Expand All @@ -88,12 +108,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 } /> }
Copy link
Member

Choose a reason for hiding this comment

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

You mentioned the clipboard button library being the cause that led to this implementation to focus text again after copying. Is that something that can be (or perhaps is already) fixed at the library? Or fixed on the clipboard button? Maybe using our "withFocusReturn" higher-order component could work as a solution?

https://github.com/WordPress/gutenberg/blob/master/components/higher-order/with-focus-return/index.js

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 think that might work, but now that I think about it, doesn't this only work on componentDidUnmount? The only react controlled component is the copy button (the textarea that gets focus is created by the clipboard library itself), and I thought the copy button wasn't being removed from the DOM after clicking on it?

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 11, 2017

Choose a reason for hiding this comment

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

There is also an option in the clipboard library of passing through trigger, and then calling clearSelection (which will focus trigger and then removes the selection), but it would be just calling focus outside of the react in the exact same way. Is that actually preferable?

<h1>
<Textarea
ref={ this.bindTextarea }
Expand Down Expand Up @@ -126,4 +148,4 @@ export default connect(
},
};
}
)( clickOutside( PostTitle ) );
)( withFocusOutside( PostTitle ) );