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

Merge the Button and IconButton into a single component #19193

Merged
merged 5 commits into from
Dec 19, 2019
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Basic rendering should display with required props 1`] = `"<span><div tabindex=\\"-1\\"><div><div><div class=\\"components-popover block-editor-link-control\\"><div class=\\"components-popover__content\\" tabindex=\\"-1\\"><div class=\\"block-editor-link-control__popover-inner\\"><div class=\\"block-editor-link-control__search\\"><form><div class=\\"components-base-control block-editor-url-input block-editor-link-control__search-input\\"><div class=\\"components-base-control__field\\"><input type=\\"text\\" aria-label=\\"URL\\" required=\\"\\" placeholder=\\"Search or type url\\" role=\\"combobox\\" aria-expanded=\\"false\\" aria-autocomplete=\\"list\\" aria-owns=\\"block-editor-url-input-suggestions-0\\" value=\\"\\"></div></div><button type=\\"reset\\" disabled=\\"\\" aria-label=\\"Reset\\" class=\\"components-button components-icon-button block-editor-link-control__search-reset\\"><svg aria-hidden=\\"true\\" role=\\"img\\" focusable=\\"false\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"20\\" height=\\"20\\" viewBox=\\"0 0 20 20\\" class=\\"dashicon dashicons-no-alt\\"><path d=\\"M14.95 6.46L11.41 10l3.54 3.54-1.41 1.41L10 11.42l-3.53 3.53-1.42-1.42L8.58 10 5.05 6.47l1.42-1.42L10 8.58l3.54-3.53z\\"></path></svg></button></form></div></div></div></div></div></div></div></span>"`;
exports[`Basic rendering should display with required props 1`] = `"<span><div tabindex=\\"-1\\"><div><div><div class=\\"components-popover block-editor-link-control\\"><div class=\\"components-popover__content\\" tabindex=\\"-1\\"><div class=\\"block-editor-link-control__popover-inner\\"><div class=\\"block-editor-link-control__search\\"><form><div class=\\"components-base-control block-editor-url-input block-editor-link-control__search-input\\"><div class=\\"components-base-control__field\\"><input type=\\"text\\" aria-label=\\"URL\\" required=\\"\\" placeholder=\\"Search or type url\\" role=\\"combobox\\" aria-expanded=\\"false\\" aria-autocomplete=\\"list\\" aria-owns=\\"block-editor-url-input-suggestions-0\\" value=\\"\\"></div></div><button type=\\"reset\\" disabled=\\"\\" class=\\"components-button block-editor-link-control__search-reset components-icon-button\\" aria-label=\\"Reset\\"><svg aria-hidden=\\"true\\" role=\\"img\\" focusable=\\"false\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"20\\" height=\\"20\\" viewBox=\\"0 0 20 20\\" class=\\"dashicon dashicons-no-alt\\"><path d=\\"M14.95 6.46L11.41 10l3.54 3.54-1.41 1.41L10 11.42l-3.53 3.53-1.42-1.42L8.58 10 5.05 6.47l1.42-1.42L10 8.58l3.54-3.53z\\"></path></svg></button></form></div></div></div></div></div></div></div></span>"`;
67 changes: 58 additions & 9 deletions packages/components/src/button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,19 @@
* External dependencies
*/
import classnames from 'classnames';
import { isArray } from 'lodash';

/**
* WordPress dependencies
*/
import deprecated from '@wordpress/deprecated';
import { createElement, forwardRef } from '@wordpress/element';
import { forwardRef } from '@wordpress/element';

/**
* Internal dependencies
*/
import Tooltip from '../tooltip';
import Icon from '../icon';

export function Button( props, ref ) {
const {
Expand All @@ -25,6 +32,13 @@ export function Button( props, ref ) {
isDestructive,
className,
disabled,
icon,
iconSize,
showTooltip,
tooltipPosition,
shortcut,
label,
children,
...additionalProps
} = props;

Expand All @@ -44,19 +58,54 @@ export function Button( props, ref ) {
'is-busy': isBusy,
'is-link': isLink,
'is-destructive': isDestructive,
'has-text': !! icon && !! children,
// Ideally should be has-icon but this is named this way for BC
Copy link
Member

Choose a reason for hiding this comment

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

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, we can do it, just trying to keep the changes small for now. Ideally, we also deprecate the component.

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 looked at this and there are a few components that rely on this for styling. I'm going to leave it for a follow-up PR to ensure we test these properly and ease review of the current PR.

'components-icon-button': !! icon,
} );

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

return createElement( tag, {
...tagProps,
...additionalProps,
className: classes,
ref,
} );
// Should show the tooltip if...
const shouldShowTooltip = ! disabled && (
// an explicit tooltip is passed or...
( showTooltip && label ) ||
// there's a shortcut or...
shortcut ||
(
// there's a label and...
!! label &&
// the children are empty and...
( ! children || ( isArray( children ) && ! children.length ) ) &&
// the tooltip is not explicitly disabled.
false !== showTooltip
Copy link
Member

Choose a reason for hiding this comment

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

this condition is now very similar to

( showTooltip && label )

we can refactor it later as it's quite hard to read :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a difference between explicit disabling of tooltips and "undefined"

Copy link
Member

Choose a reason for hiding this comment

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

I figured it out after 2 additional minutes spend on contemplating on it 🤣

)
);

const element = (
<Tag
{ ...tagProps }
{ ...additionalProps }
className={ classes }
aria-label={ additionalProps[ 'aria-label' ] || label }
ref={ ref }
>
{ icon && <Icon icon={ icon } size={ iconSize } /> }
{ children }
</Tag>
);

if ( ! shouldShowTooltip ) {
return element;
}

return (
<Tooltip text={ label } shortcut={ shortcut } position={ tooltipPosition }>
{ element }
</Tooltip>
);
}

export default forwardRef( Button );
17 changes: 17 additions & 0 deletions packages/components/src/button/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,23 @@
}
}

.components-icon-button {
.dashicon {
display: inline-block;
flex: 0 0 auto;
}

// Ensure that even SVG icons that don't include the .dashicon class are colored.
svg {
fill: currentColor;
outline: none;
}

&.has-text svg {
margin-right: 8px;
}
}

@keyframes components-button__busy-animation {
0% {
background-position: 200px 0;
Expand Down
51 changes: 51 additions & 0 deletions packages/components/src/button/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,57 @@ describe( 'Button', () => {

expect( button.prop( 'WordPress' ) ).toBe( 'awesome' );
} );

it( 'should render an icon button', () => {
const iconButton = shallow( <Button icon="plus" /> );
expect( iconButton.hasClass( 'components-icon-button' ) ).toBe( true );
expect( iconButton.prop( 'aria-label' ) ).toBeUndefined();
} );

it( 'should render a Dashicon component matching the wordpress icon', () => {
const iconButton = shallow( <Button icon="wordpress" /> );
expect( iconButton.find( 'Icon' ).dive().shallow().hasClass( 'dashicons-wordpress' ) ).toBe( true );
} );

it( 'should render child elements and icon', () => {
const iconButton = shallow( <Button icon="wordpress" children={ <p className="test">Test</p> } /> );
expect( iconButton.find( 'Icon' ).dive().shallow().hasClass( 'dashicons-wordpress' ) ).toBe( true );
expect( iconButton.find( '.test' ).shallow().text() ).toBe( 'Test' );
} );

it( 'should add an aria-label when the label property is used, with Tooltip wrapper', () => {
const iconButton = shallow( <Button icon="WordPress" label="WordPress" /> );
expect( iconButton.name() ).toBe( 'Tooltip' );
expect( iconButton.prop( 'text' ) ).toBe( 'WordPress' );
expect( iconButton.find( 'button' ).prop( 'aria-label' ) ).toBe( 'WordPress' );
} );

it( 'should support explicit aria-label override', () => {
const iconButton = shallow( <Button aria-label="Custom" /> );
expect( iconButton.prop( 'aria-label' ) ).toBe( 'Custom' );
} );

it( 'should allow tooltip disable', () => {
const iconButton = shallow( <Button icon="WordPress" label="WordPress" showTooltip={ false } /> );
expect( iconButton.name() ).toBe( 'button' );
expect( iconButton.prop( 'aria-label' ) ).toBe( 'WordPress' );
} );

it( 'should show the tooltip for empty children', () => {
const iconButton = shallow( <Button icon="WordPress" label="WordPress" children={ [] } /> );
expect( iconButton.name() ).toBe( 'Tooltip' );
expect( iconButton.prop( 'text' ) ).toBe( 'WordPress' );
} );

it( 'should not show the tooltip when icon and children defined', () => {
const iconButton = shallow( <Button icon="WordPress" label="WordPress">Children</Button> );
expect( iconButton.name() ).toBe( 'button' );
} );

it( 'should force showing the tooltip even if icon and children defined', () => {
const iconButton = shallow( <Button icon="WordPress" label="WordPress" showTooltip>Children</Button> );
expect( iconButton.name() ).toBe( 'Tooltip' );
} );
} );

describe( 'with href property', () => {
Expand Down
73 changes: 14 additions & 59 deletions packages/components/src/icon-button/index.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
/**
* External dependencies
*/
import classnames from 'classnames';
import { isArray } from 'lodash';

/**
* WordPress dependencies
*/
Expand All @@ -12,64 +6,25 @@ import { forwardRef } from '@wordpress/element';
/**
* Internal dependencies
*/
import Tooltip from '../tooltip';
import Button from '../button';
import Icon from '../icon';

function IconButton( props, ref ) {
const {
icon,
children,
label,
className,
tooltip,
shortcut,
labelPosition,
size,
...additionalProps
} = props;
const classes = classnames( 'components-icon-button', className, {
'has-text': children,
} );
const tooltipText = tooltip || label;

// Should show the tooltip if...
const showTooltip = ! additionalProps.disabled && (
// an explicit tooltip is passed or...
tooltip ||
// there's a shortcut or...
shortcut ||
(
// there's a label and...
!! label &&
// the children are empty and...
( ! children || ( isArray( children ) && ! children.length ) ) &&
// the tooltip is not explicitly disabled.
false !== tooltip
)
);

let element = (
function IconButton( {
labelPosition,
size,
tooltip,
label,
...props
}, ref ) {
return (
<Button
aria-label={ label }
{ ...additionalProps }
className={ classes }
{ ...props }
ref={ ref }
>
<Icon icon={ icon } size={ size } />
{ children }
</Button>
tooltipPosition={ labelPosition }
iconSize={ size }
showTooltip={ tooltip !== undefined ? !! tooltip : undefined }
label={ tooltip || label }
/>
);

if ( showTooltip ) {
element = (
<Tooltip text={ tooltipText } shortcut={ shortcut } position={ labelPosition }>
{ element }
</Tooltip>
);
}

return element;
}

export default forwardRef( IconButton );
16 changes: 0 additions & 16 deletions packages/components/src/icon-button/style.scss

This file was deleted.

91 changes: 0 additions & 91 deletions packages/components/src/icon-button/test/index.js

This file was deleted.

1 change: 0 additions & 1 deletion packages/components/src/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
@import "./form-token-field/style.scss";
@import "./guide/style.scss";
@import "./higher-order/navigate-regions/style.scss";
@import "./icon-button/style.scss";
@import "./menu-group/style.scss";
@import "./menu-item/style.scss";
@import "./menu-items-choice/style.scss";
Expand Down
Loading