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

Components: Implement Button as assigning ref via forwardRef #6527

Merged
merged 2 commits into from
May 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
92 changes: 36 additions & 56 deletions components/button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,67 +6,47 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { Component, createElement } from '@wordpress/element';
import { createElement, forwardRef } from '@wordpress/element';

/**
* Internal dependencies
*/
import './style.scss';

class Button extends Component {
constructor( props ) {
super( props );
this.setRef = this.setRef.bind( this );
}

componentDidMount() {
if ( this.props.focus ) {
this.ref.focus();
}
}

setRef( ref ) {
this.ref = ref;
}

focus() {
this.ref.focus();
}

render() {
const {
href,
target,
isPrimary,
isLarge,
isSmall,
isToggled,
isBusy,
className,
disabled,
...additionalProps
} = this.props;
const classes = classnames( 'components-button', className, {
button: ( isPrimary || isLarge || isSmall ),
'button-primary': isPrimary,
'button-large': isLarge,
'button-small': isSmall,
'is-toggled': isToggled,
'is-busy': isBusy,
} );

const tag = href !== undefined && ! disabled ? 'a' : 'button';
const tagProps = tag === 'a' ? { href, target } : { type: 'button', disabled };

delete additionalProps.focus;

return createElement( tag, {
...tagProps,
...additionalProps,
className: classes,
ref: this.setRef,
} );
}
export function Button( props, ref ) {
const {
href,
target,
isPrimary,
isLarge,
isSmall,
isToggled,
isBusy,
className,
disabled,
focus,
...additionalProps
} = props;

const classes = classnames( 'components-button', className, {
button: ( isPrimary || isLarge || isSmall ),
'button-primary': isPrimary,
'button-large': isLarge,
'button-small': isSmall,
'is-toggled': isToggled,
'is-busy': isBusy,
} );

const tag = href !== undefined && ! disabled ? 'a' : 'button';
const tagProps = tag === 'a' ? { href, target } : { type: 'button', disabled };

return createElement( tag, {
...tagProps,
...additionalProps,
className: classes,
autoFocus: focus,
ref,
} );
}

export default Button;
export default forwardRef( Button );
26 changes: 24 additions & 2 deletions components/button/test/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';
import { mount, shallow } from 'enzyme';

/**
* WordPress dependencies
*/
import { createRef } from '@wordpress/element';

/**
* Internal dependencies
*/
import Button from '../';
import ButtonWithForwardedRef, { Button } from '../';

// [TEMPORARY]: Only needed so long as Enzyme does not support React.forwardRef
jest.unmock( '../' );

describe( 'Button', () => {
describe( 'basic rendering', () => {
Expand Down Expand Up @@ -93,4 +101,18 @@ describe( 'Button', () => {
expect( button.type() ).toBe( 'button' );
} );
} );

// Disable reason: This test is desirable, but unsupported by Enzyme in
// the current version, as it depends on features new to React in 16.3.0.
//
// eslint-disable-next-line jest/no-disabled-tests
Copy link
Member

Choose a reason for hiding this comment

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

Should we e.g. create an issue to remind us to unskip this in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we e.g. create an issue to remind us to unskip this in the future?

Created at #7005

describe.skip( 'ref forwarding', () => {
it( 'should enable access to DOM element', () => {
const ref = createRef();

mount( <ButtonWithForwardedRef ref={ ref } /> );

expect( ref.current.nodeName ).toBe( 'button' );
Copy link
Member Author

Choose a reason for hiding this comment

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

This test will fail when unskipped: Element#nodeName returns uppercase, so should be asserting as toBe( 'BUTTON' );

Copy link
Member

Choose a reason for hiding this comment

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

It may take another few weeks until we can unskip those tests :(
The day when we remove Enzyme is becoming a reality if there is no progress on supporting React 16.3+

} );
} );
} );
14 changes: 14 additions & 0 deletions element/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
createElement,
createContext,
createRef,
forwardRef,
Component,
cloneElement,
Children,
Expand Down Expand Up @@ -51,6 +52,19 @@ export { createElement };
*/
export { createRef };

/**
* Component enhancer used to enable passing a ref to its wrapped component.
* Pass a function argument which receives `props` and `ref` as its arguments,
* returning an element using the forwarded ref. The return value is a new
* component which forwards its ref.
*
* @param {Function} forwarder Function passed `props` and `ref`, expected to
* return an element.
*
* @return {WPComponent} Enhanced component.
*/
export { forwardRef };

/**
* Renders a given element into the target DOM node.
*
Expand Down
3 changes: 2 additions & 1 deletion test/unit/jest.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
"setupFiles": [
"core-js/fn/symbol/async-iterator",
"<rootDir>/test/unit/setup-blocks.js",
"<rootDir>/test/unit/setup-wp-aliases.js"
"<rootDir>/test/unit/setup-wp-aliases.js",
"<rootDir>/test/unit/setup-mocks.js"
],
"transform": {
"\\.pegjs$": "<rootDir>/test/unit/pegjs-transform.js"
Expand Down
16 changes: 16 additions & 0 deletions test/unit/setup-mocks.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// [TEMPORARY]: Button uses React.forwardRef, added in [email protected] but not yet
// supported by Enzyme as of [email protected] . This mock unwraps
// the ref forwarding, so any tests relying on this behavior will fail.
//
// See: https://github.com/airbnb/enzyme/issues/1604
// See: https://github.com/airbnb/enzyme/pull/1592/files
jest.mock( '../../components/button', () => {
const { Button: RawButton } = require.requireActual( '../../components/button' );
const { Component } = require( 'react' );

return class Button extends Component {
render() {
return RawButton( this.props );
}
};
} );