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

feat: update fundamental-styles to 0.12.0-rc.44 #1203

Merged
merged 13 commits into from
Sep 15, 2020
6 changes: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"@popperjs/core": "^2.4.3",
"chain-function": "^1.0.1",
"classnames": "^2.2.6",
"fundamental-styles": "0.12.0-rc.18",
"fundamental-styles": "0.12.0-rc.44",
"keycode": "^2.2.0",
"moment": "^2.26.0",
"prop-types": "^15.7.1",
Expand Down
2 changes: 1 addition & 1 deletion src/ActionBar/ActionBar.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('<ActionBar />', () => {
});

expect(
element.find('.sap-icon--navigation-left-arrow').getDOMNode().attributes['data-sample'].value
element.find('button.fd-button--transparent').getDOMNode().attributes['data-sample'].value
).toBe('Sample');
});

Expand Down
78 changes: 64 additions & 14 deletions src/ActionBar/__stories__/__snapshots__/ActionBar.stories.storyshot
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,16 @@ exports[`Storyshots Component API/ActionBar Dev 1`] = `
>
<button
aria-label="Back to home"
className="fd-button fd-button--transparent fd-button--compact sap-icon--navigation-left-arrow"
className="fd-button fd-button--transparent fd-button--compact"
onClick={[Function]}
type="button"
/>
>
<i
aria-hidden={true}
className="sap-icon--navigation-left-arrow"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do all these icons need aria-hidden attributes like we do in widgets?

role="img"
/>
</button>
</div>
<div
className="fd-action-bar__title"
Expand All @@ -36,13 +42,21 @@ exports[`Storyshots Component API/ActionBar Dev 1`] = `
className="fd-button"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
<button
className="fd-button fd-button--emphasized"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
</div>
</div>
Expand Down Expand Up @@ -105,13 +119,21 @@ exports[`Storyshots Component API/ActionBar No Back Button 1`] = `
className="fd-button"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
<button
className="fd-button fd-button--emphasized"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
</div>
</div>
Expand Down Expand Up @@ -150,13 +172,21 @@ exports[`Storyshots Component API/ActionBar No Description 1`] = `
className="fd-button"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
<button
className="fd-button fd-button--emphasized"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
</div>
</div>
Expand All @@ -179,10 +209,16 @@ exports[`Storyshots Component API/ActionBar Primary 1`] = `
>
<button
aria-label="Back to home"
className="fd-button fd-button--transparent fd-button--compact sap-icon--navigation-left-arrow"
className="fd-button fd-button--transparent fd-button--compact"
onClick={[Function]}
type="button"
/>
>
<i
aria-hidden={true}
className="sap-icon--navigation-left-arrow"
role="img"
/>
</button>
</div>
<div
className="fd-action-bar__title"
Expand All @@ -200,13 +236,21 @@ exports[`Storyshots Component API/ActionBar Primary 1`] = `
className="fd-button"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
<button
className="fd-button fd-button--emphasized"
type="button"
>
Button
<span
className="fd-button__text"
>
Button
</span>
</button>
</div>
</div>
Expand Down Expand Up @@ -257,13 +301,19 @@ exports[`Storyshots Component API/ActionBar With a ContextualMenu 1`] = `
aria-expanded={false}
aria-haspopup={true}
aria-label="More actions"
className="fd-button fd-button--transparent sap-icon--vertical-grip"
className="fd-button fd-button--transparent"
onClick={[Function]}
onKeyPress={[Function]}
role="button"
tabIndex={0}
type="button"
/>
>
<i
aria-hidden={true}
className="sap-icon--vertical-grip"
role="img"
/>
</button>
</div>
</div>
</div>
Expand Down
35 changes: 31 additions & 4 deletions src/Button/Button.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import classnames from 'classnames';
import Icon from '../Icon/Icon';
import PropTypes from 'prop-types';
import React from 'react';
import useUniqueId from '../utils/useUniqueId';
import { BUTTON_OPTIONS, BUTTON_TYPES } from '../utils/constants';
import 'fundamental-styles/dist/icon.css';
import 'fundamental-styles/dist/button.css';

/** A **Button** allows users to perform an action. The priority of buttons within a page should be considered.
Expand All @@ -19,12 +19,15 @@ const Button = React.forwardRef(({
disabledMessage,
enabledMessage,
glyph,
iconBeforeText,
iconProps,
selected,
disabled,
typeAttr,
onClick,
children,
className,
textClassName,
...props
}, ref) => {

Expand All @@ -34,13 +37,17 @@ const Button = React.forwardRef(({
[`fd-button--${option}`]: !!option,
[`fd-button--${type}`]: !!type,
'fd-button--compact': compact,
[`sap-icon--${glyph}`]: !!glyph,
'is-selected': selected,
'is-disabled': disabled
},
className
);

const buttonTextClasses = classnames(
'fd-button__text',
textClassName
);

const ariaDisabled = props['aria-disabled'];
let disabledInAnyWay = disabled || ariaDisabled;

Expand Down Expand Up @@ -76,6 +83,18 @@ const Button = React.forwardRef(({
}
return content;
};

const renderIcon = () => {
const content = glyph ? (
<Icon
{...iconProps}
ariaHidden
glyph={glyph} />
) : null;

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work to just do something like:

const renderIcon = () => {
return ( glyph && <Icon props here />
)

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lint complains Declare only one React component per file

Not sure if it's cleaner to disable it / if we want to 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, I think it's fine the way you have it

return content;
};

return (
<>
<button
Expand All @@ -86,7 +105,9 @@ const Button = React.forwardRef(({
ref={ref}
selected={selected}
type={typeAttr}>
{children}
{iconBeforeText && renderIcon()}
{children && <span className={buttonTextClasses}>{children}</span>}
{!iconBeforeText && renderIcon()}
</button>
{renderButtonStateMessage()}
</>
Expand Down Expand Up @@ -134,10 +155,16 @@ Button.propTypes = {
enabledMessage: validateStateTransitionMessage,
/** The icon to include. See the icon page for the list of icons */
glyph: PropTypes.string,
/** Indicates the importance of the button: 'empahsized' or 'transparent' */
/** Determines whether the icon should be placed before the text */
iconBeforeText: PropTypes.bool,
/** Additional props to be spread to the Icon component */
iconProps: PropTypes.object,
/** Indicates the importance of the button: 'emphasized' or 'transparent' */
option: PropTypes.oneOf(BUTTON_OPTIONS),
/** Set to **true** to set state of the button to "selected" */
selected: PropTypes.bool,
/** CSS class(es) to add to the text element */
textClassName: PropTypes.string,
/** Sets the variation of the component. Primarily used for styling: 'standard',
'positive',
'negative',
Expand Down
32 changes: 32 additions & 0 deletions src/Button/Button.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@ describe('<Button />', () => {
element.find('.fd-button').getDOMNode().attributes['data-sample'].value
).toBe('Sample');
});

test('should allow spreading className to inner text', () => {
const element = mount(<Button textClassName='wonderful-styles'>Button</Button>);

expect(
element.find('.fd-button__text').getDOMNode().classList
).toContain('wonderful-styles');
});

test('should allow spreading props to icon', () => {
const element = mount(<Button glyph='bell'iconProps={{ 'data-sample': 'Sample' }}>Button</Button>);

expect(
element.find('i.sap-icon--bell').getDOMNode().attributes['data-sample'].value
).toBe('Sample');
});
});
test('forwards the ref', () => {
let ref;
Expand All @@ -26,6 +42,22 @@ describe('<Button />', () => {
expect(ref.current.className).toEqual('fd-button');
});

test('should render the icon before the text when `iconBeforeText` is true', () => {
const element = mount(<Button glyph='bell' iconBeforeText>Button</Button>);

expect(
element.find('.fd-button').childAt(0).getDOMNode().classList
).toContain('sap-icon--bell');
});

test('should render the icon after the text when `iconBeforeText` is not defined', () => {
const element = mount(<Button glyph='bell'>Button</Button>);

expect(
element.find('.fd-button').childAt(1).getDOMNode().classList
).toContain('sap-icon--bell');
});

describe('a11y', () => {
const element = mount(
<Button allowFocusOnDisable disabled
Expand Down
5 changes: 3 additions & 2 deletions src/Button/__stories__/Button.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export const types = () => (

types.storyName = 'Types';

/** Button can have an icon with text or just and icon. */
/** Button can have an icon with text or just an icon. Be sure to use an `aria-label` if there is no text. */

Copy link
Member

Choose a reason for hiding this comment

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

Can we add aria-label to the

<Button glyph='alert' option='emphasized' />

in this file.

export const icons = () => (
<div className='fddocs-container'>
Expand All @@ -65,7 +65,8 @@ export const icons = () => (
<Button glyph='edit' type='ghost'>Edit</Button>
<Button glyph='warning' type='attention'>Ignore</Button>
<div className='fddocs-container--break' />
<Button glyph='alert' option='emphasized' />
<Button aria-label='See warning' glyph='alert'
option='emphasized' />
<Button aria-label='Add to cart' glyph='cart' />
<Button aria-label='Filter' glyph='cart'
option='transparent' />
Expand Down
Loading