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

Jump to bottom

Make IconButton able to be referenced. #14163

Merged
merged 3 commits into from
Mar 1, 2019
Merged

Make IconButton able to be referenced. #14163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2c21db
Make IconButton able to get referenced.
afercia Feb 28, 2019
c050b67
Components: Forward IconButton ref as stateless function
aduth Feb 28, 2019
6b081a3
Components: Restore IconButton aria-pressed prop pass-through
aduth Mar 1, 2019
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
2 changes: 1 addition & 1 deletion packages/components/src/form-file-upload/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe( 'InserterMenu', () => {
</FormFileUpload>
);

const iconButton = wrapper.find( 'IconButton' );
const iconButton = wrapper.find( 'ForwardRef(IconButton)' );
const input = wrapper.find( 'input' );
expect( iconButton.prop( 'children' ) ).toBe( 'My Upload Button' );
expect( input.prop( 'style' ).display ).toBe( 'none' );
Expand Down
30 changes: 25 additions & 5 deletions packages/components/src/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { isArray, isString } from 'lodash';
/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { Component, forwardRef } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -18,9 +18,19 @@ import Dashicon from '../dashicon';

// This is intentionally a Component class, not a function component because it
Copy link
Member

Choose a reason for hiding this comment

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

From the comment here, and the introduction of forwardRef, I wonder if it's still necessary for this to be a class. Having it be a function instead simplifies greatly the ref forwarding, since it's provided as the second argument to the function (see button/index.js).

Let me test to see if that'll work...

// is common to apply a ref to the button element (only supported in class)
class IconButton extends Component {
export class IconButton extends Component {
render() {
const { icon, children, label, className, tooltip, shortcut, labelPosition, ...additionalProps } = this.props;
const {
icon,
children,
label,
className,
tooltip,
shortcut,
labelPosition,
forwardedRef,
...additionalProps
} = this.props;
const { 'aria-pressed': ariaPressed } = this.props;
const classes = classnames( 'components-icon-button', className, {
'has-text': children,
Expand All @@ -44,7 +54,12 @@ class IconButton extends Component {
);

let element = (
<Button aria-label={ label } { ...additionalProps } className={ classes }>
<Button
aria-label={ label }
{ ...additionalProps }
className={ classes }
ref={ forwardedRef }
>
{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } /> : icon }
{ children }
</Button>
Expand All @@ -62,4 +77,9 @@ class IconButton extends Component {
}
}

export default IconButton;
const forwardedIconButton = ( props, ref ) => {
return <IconButton { ...props } forwardedRef={ ref } />;
};
forwardedIconButton.displayName = 'IconButton';

export default forwardRef( forwardedIconButton );
2 changes: 1 addition & 1 deletion packages/components/src/icon-button/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { shallow } from 'enzyme';
/**
* Internal dependencies
*/
import IconButton from '../';
import { IconButton } from '../';

describe( 'IconButton', () => {
describe( 'basic rendering', () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/menu-item/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`MenuItem should match snapshot when all props provided 1`] = `
<IconButton
<ForwardRef(IconButton)
aria-checked={true}
className="components-menu-item__button my-class has-icon"
icon="wordpress"
Expand All @@ -13,7 +13,7 @@ exports[`MenuItem should match snapshot when all props provided 1`] = `
className="components-menu-item__shortcut"
shortcut="mod+shift+alt+w"
/>
</IconButton>
</ForwardRef(IconButton)>
`;

exports[`MenuItem should match snapshot when info is provided 1`] = `
Expand All @@ -40,7 +40,7 @@ exports[`MenuItem should match snapshot when info is provided 1`] = `
`;

exports[`MenuItem should match snapshot when isSelected and role are optionally provided 1`] = `
<IconButton
<ForwardRef(IconButton)
className="components-menu-item__button my-class has-icon"
icon="wordpress"
onClick={[Function]}
Expand All @@ -51,7 +51,7 @@ exports[`MenuItem should match snapshot when isSelected and role are optionally
className="components-menu-item__shortcut"
shortcut="mod+shift+alt+w"
/>
</IconButton>
</ForwardRef(IconButton)>
`;

exports[`MenuItem should match snapshot when only label provided 1`] = `
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/notice/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ exports[`Notice should match snapshot 1`] = `
View
</ForwardRef(Button)>
</div>
<IconButton
<ForwardRef(IconButton)
className="components-notice__dismiss"
icon="no"
label="Dismiss this notice"
Expand Down
93 changes: 51 additions & 42 deletions packages/edit-post/src/components/header/more-menu/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,54 +12,52 @@ exports[`MoreMenu should match snapshot 1`] = `
<div
className="edit-post-more-menu"
>
<IconButton
<ForwardRef(IconButton)
aria-expanded={false}
icon="ellipsis"
label="Show more tools & options"
labelPosition="bottom"
onClick={[Function]}
>
<Tooltip
position="bottom"
text="Show more tools & options"
<IconButton
aria-expanded={false}
forwardedRef={null}
icon="ellipsis"
label="Show more tools & options"
labelPosition="bottom"
onClick={[Function]}
>
<ForwardRef(Button)
aria-expanded={false}
aria-label="Show more tools & options"
className="components-icon-button"
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
<Tooltip
position="bottom"
text="Show more tools & options"
>
<button
<ForwardRef(Button)
aria-expanded={false}
aria-label="Show more tools & options"
className="components-button components-icon-button"
className="components-icon-button"
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
>
<Dashicon
icon="ellipsis"
key="0,0"
<button
aria-expanded={false}
aria-label="Show more tools & options"
className="components-button components-icon-button"
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
>
<SVG
aria-hidden={true}
className="dashicon dashicons-ellipsis"
focusable="false"
height={20}
role="img"
viewBox="0 0 20 20"
width={20}
xmlns="http://www.w3.org/2000/svg"
<Dashicon
icon="ellipsis"
key="0,0"
>
<svg
aria-hidden="true"
<SVG
aria-hidden={true}
className="dashicon dashicons-ellipsis"
focusable="false"
height={20}
Expand All @@ -68,20 +66,31 @@ exports[`MoreMenu should match snapshot 1`] = `
width={20}
xmlns="http://www.w3.org/2000/svg"
>
<Path
d="M5 10c0 1.1-.9 2-2 2s-2-.9-2-2 .9-2 2-2 2 .9 2 2zm12-2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2zm-7 0c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z"
<svg
aria-hidden="true"
className="dashicon dashicons-ellipsis"
focusable="false"
height={20}
role="img"
viewBox="0 0 20 20"
width={20}
xmlns="http://www.w3.org/2000/svg"
>
<path
<Path
d="M5 10c0 1.1-.9 2-2 2s-2-.9-2-2 .9-2 2-2 2 .9 2 2zm12-2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2zm-7 0c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z"
/>
</Path>
</svg>
</SVG>
</Dashicon>
</button>
</ForwardRef(Button)>
</Tooltip>
</IconButton>
>
<path
d="M5 10c0 1.1-.9 2-2 2s-2-.9-2-2 .9-2 2-2 2 .9 2 2zm12-2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2zm-7 0c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z"
/>
</Path>
</svg>
</SVG>
</Dashicon>
</button>
</ForwardRef(Button)>
</Tooltip>
</IconButton>
</ForwardRef(IconButton)>
</div>
</Dropdown>
</MoreMenu>
Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/block-mover/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ describe( 'BlockMover', () => {
const moveDown = blockMover.childAt( 2 );
const moveUpDesc = blockMover.childAt( 3 );
const moveDownDesc = blockMover.childAt( 4 );
expect( moveUp.type().name ).toBe( 'IconButton' );
expect( moveUp.name() ).toBe( 'ForwardRef(IconButton)' );
expect( drag.type().name ).toBe( 'IconDragHandle' );
expect( moveDown.type().name ).toBe( 'IconButton' );
expect( moveDown.name() ).toBe( 'ForwardRef(IconButton)' );
expect( moveUp.props() ).toMatchObject( {
className: 'editor-block-mover__control',
onClick: undefined,
Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/block-switcher/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`BlockSwitcher should render disabled block switcher with multi block of different types when no transforms 1`] = `
<Toolbar>
<IconButton
<ForwardRef(IconButton)
className="editor-block-switcher__no-switcher-icon"
disabled={true}
label="Block icon"
Expand All @@ -11,7 +11,7 @@ exports[`BlockSwitcher should render disabled block switcher with multi block of
icon="layout"
showColors={true}
/>
</IconButton>
</ForwardRef(IconButton)>
</Toolbar>
`;

Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/block-switcher/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ describe( 'BlockSwitcher', () => {

test( 'should simulate a keydown event, which should call onToggle and open transform toggle.', () => {
const toggleClosed = shallow( getDropdown().props().renderToggle( { onToggle: onToggleStub, isOpen: false } ) );
const iconButtonClosed = toggleClosed.find( 'IconButton' );
const iconButtonClosed = toggleClosed.find( 'ForwardRef(IconButton)' );

iconButtonClosed.simulate( 'keydown', mockKeyDown );

Expand All @@ -180,7 +180,7 @@ describe( 'BlockSwitcher', () => {

test( 'should simulate a click event, which should call onToggle.', () => {
const toggleOpen = shallow( getDropdown().props().renderToggle( { onToggle: onToggleStub, isOpen: true } ) );
const iconButtonOpen = toggleOpen.find( 'IconButton' );
const iconButtonOpen = toggleOpen.find( 'ForwardRef(IconButton)' );

iconButtonOpen.simulate( 'keydown', mockKeyDown );

Expand Down
10 changes: 5 additions & 5 deletions packages/editor/src/components/post-publish-panel/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is pu
>
Published
</div>
<IconButton
<ForwardRef(IconButton)
aria-expanded={true}
icon="no-alt"
label="Close panel"
Expand Down Expand Up @@ -47,7 +47,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is sc
>
Scheduled
</div>
<IconButton
<ForwardRef(IconButton)
aria-expanded={true}
icon="no-alt"
label="Close panel"
Expand Down Expand Up @@ -88,7 +88,7 @@ exports[`PostPublishPanel should render the pre-publish panel if post status is
className="editor-post-publish-panel__spacer"
/>
</div>
<IconButton
<ForwardRef(IconButton)
aria-expanded={true}
icon="no-alt"
label="Close panel"
Expand Down Expand Up @@ -127,7 +127,7 @@ exports[`PostPublishPanel should render the pre-publish panel if the post is not
className="editor-post-publish-panel__spacer"
/>
</div>
<IconButton
<ForwardRef(IconButton)
aria-expanded={true}
icon="no-alt"
label="Close panel"
Expand Down Expand Up @@ -166,7 +166,7 @@ exports[`PostPublishPanel should render the spinner if the post is being saved 1
className="editor-post-publish-panel__spacer"
/>
</div>
<IconButton
<ForwardRef(IconButton)
aria-expanded={true}
icon="no-alt"
label="Close panel"
Expand Down
6 changes: 3 additions & 3 deletions packages/editor/src/components/url-input/test/button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,19 @@ import URLInput from '../';
import URLInputButton from '../button';

describe( 'URLInputButton', () => {
const clickEditLink = ( wrapper ) => wrapper.find( 'IconButton.components-toolbar__control' ).simulate( 'click' );
const clickEditLink = ( wrapper ) => wrapper.find( 'ForwardRef(IconButton).components-toolbar__control' ).simulate( 'click' );

it( 'should have a valid class name in the wrapper tag', () => {
const wrapper = shallow( <URLInputButton /> );
expect( wrapper.hasClass( 'editor-url-input__button' ) ).toBe( true );
} );
it( 'should not have is-active class when url prop not defined', () => {
const wrapper = shallow( <URLInputButton /> );
expect( wrapper.find( 'IconButton' ).hasClass( 'is-active' ) ).toBe( false );
expect( wrapper.find( 'ForwardRef(IconButton)' ).hasClass( 'is-active' ) ).toBe( false );
} );
it( 'should have is-active class name if url prop defined', () => {
const wrapper = shallow( <URLInputButton url="https://example.com" /> );
expect( wrapper.find( 'IconButton' ).hasClass( 'is-active' ) ).toBe( true );
expect( wrapper.find( 'ForwardRef(IconButton)' ).hasClass( 'is-active' ) ).toBe( true );
} );
it( 'should have hidden form by default', () => {
const wrapper = shallow( <URLInputButton /> );
Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/url-popover/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ exports[`URLPopover matches the snapshot in its default state 1`] = `
<div>
Editor
</div>
<IconButton
<ForwardRef(IconButton)
aria-expanded={false}
className="editor-url-popover__settings-toggle"
icon="arrow-down-alt2"
Expand All @@ -37,7 +37,7 @@ exports[`URLPopover matches the snapshot when the settings are toggled open 1`]
<div>
Editor
</div>
<IconButton
<ForwardRef(IconButton)
aria-expanded={true}
className="editor-url-popover__settings-toggle"
icon="arrow-down-alt2"
Expand Down
2 changes: 1 addition & 1 deletion packages/nux/src/components/dot-tip/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ exports[`DotTip should render correctly 1`] = `
Got it
</ForwardRef(Button)>
</p>
<IconButton
<ForwardRef(IconButton)
className="nux-dot-tip__disable"
icon="no-alt"
label="Disable tips"
Expand Down
2 changes: 1 addition & 1 deletion packages/nux/src/components/dot-tip/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe( 'DotTip', () => {
It looks like you’re writing a letter. Would you like help?
</DotTip>
);
wrapper.find( 'IconButton[label="Disable tips"]' ).first().simulate( 'click' );
wrapper.find( 'ForwardRef(IconButton)[label="Disable tips"]' ).first().simulate( 'click' );
expect( onDisable ).toHaveBeenCalled();
} );
} );