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
31 changes: 18 additions & 13 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 @@ -20,7 +20,7 @@ const Button = React.forwardRef(({
enabledMessage,
glyph,
iconBeforeText,
iconClassName,
iconProps,
selected,
disabled,
typeAttr,
Expand Down Expand Up @@ -48,13 +48,6 @@ const Button = React.forwardRef(({
textClassName
);

const iconClasses = classnames(
{
[`sap-icon--${glyph}`]: !!glyph
},
iconClassName
);

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

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

const renderIcon = () => {
const content = glyph ? (
<Icon
{...iconProps}
ariaHidden
glyph={glyph}
isInButton />
) : 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 @@ -101,9 +106,9 @@ const Button = React.forwardRef(({
ref={ref}
selected={selected}
type={typeAttr}>
{iconBeforeText && glyph && <i aria-hidden='true' className={iconClasses} />}
{iconBeforeText && renderIcon()}
{children && <span className={buttonTextClasses}>{children}</span>}
{!iconBeforeText && glyph && <i aria-hidden='true' className={iconClasses} />}
{!iconBeforeText && renderIcon()}
</button>
{renderButtonStateMessage()}
</>
Expand Down Expand Up @@ -153,8 +158,8 @@ Button.propTypes = {
glyph: PropTypes.string,
/** Determines whether the icon should be placed before the text */
iconBeforeText: PropTypes.bool,
/** CSS class(es) to add to the icon element */
iconClassName: PropTypes.string,
/** 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" */
Expand Down
8 changes: 4 additions & 4 deletions src/Button/Button.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ describe('<Button />', () => {
).toContain('wonderful-styles');
});

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

expect(
element.find('.sap-icon--bell').getDOMNode().classList
).toContain('wonderful-styles');
element.find('i.sap-icon--bell').getDOMNode().attributes['data-sample'].value
).toBe('Sample');
});
});
test('forwards the ref', () => {
Expand Down
22 changes: 20 additions & 2 deletions src/Icon/Icon.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import 'fundamental-styles/dist/icon.css';
and focus, and for fun. Icons can be used adaptively if desired, but at
this point they are used more as visual elements within other
components. */
const Icon = React.forwardRef(({ glyph, size, className, ...props }, ref) => {
const Icon = React.forwardRef(({ ariaHidden, ariaLabel, glyph, size, className, isInButton, ...props }, ref) => {

const iconClasses = classnames(
{
Expand All @@ -22,9 +22,13 @@ const Icon = React.forwardRef(({ glyph, size, className, ...props }, ref) => {
style = { fontSize: ICON_SIZES[size] };
}

const Tag = isInButton ? 'i' : 'span';
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 was hesitant to use the Icon component because it uses a span instead of i. Should we go back to fundamental-styles so that all icons use an i as in nui-widgets?

Copy link
Contributor

Choose a reason for hiding this comment

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

After the discussion this morning, I think they're in the process of moving all icons to their own <i> tag- SAP/fundamental-styles#1604

We can probably just go ahead and use i tags since it doesn't cause any styling issues and then styles will catch up. Up to you though

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 If we could use the Icon component and use i tags to get ahead of it where possible that would save us time down the line.


return (
<span
<Tag
{...props}
aria-hidden={ariaHidden}
aria-label={ariaLabel}
className={iconClasses}
ref={ref}
style={style} />
Expand All @@ -36,8 +40,22 @@ Icon.displayName = 'Icon';
Icon.propTypes = {
/** The icon to include. See the icon page for the list of icons */
glyph: PropTypes.string.isRequired,
/** The value for the `aria-hidden` attribute. Set to `true` when there is already visible text explaining the icon, or the icon is purely for decoration. */
ariaHidden: PropTypes.bool,
/** Text to describe the icon. The value is placed in the `aria-label` attribute. */

ariaLabel: (props) => {
if (
(!props.ariaHidden && !props.ariaLabel) ||
(!props.ariaHidden && typeof props.ariaLabel !== 'string')
) {
return new Error('ariaLabel is required if ariaHidden is false and must be a string');
}
},
/** CSS class(es) to add to the element */
className: PropTypes.string,
/** If the icon is in a button */
isInButton: PropTypes.bool,
/** Size of the component: 's', 'm', 'l', 'xl' */
size: PropTypes.oneOf(Object.keys(ICON_SIZES))
};
Expand Down
30 changes: 20 additions & 10 deletions src/Icon/Icon.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@ import React from 'react';

describe('<Icon />', () => {
const mockOnClick = jest.fn();
const defaultIcon = (
<Icon
className='blue'
glyph='cart'
onClick={mockOnClick} />
);
const iconWithSize = <Icon glyph='cart' size='s' />;

const defaultProps = { ariaHidden: true, glyph: 'cart' };
const defaultIcon = <Icon {...defaultProps} />;
const clickIcon = <Icon {...defaultProps} onClick={mockOnClick} />;
const iconWithSize = <Icon {...defaultProps} size='s' />;
const ariaLabelIcon = <Icon ariaLabel='Add to cart' glyph='cart' />;
const propSpreadIcon = <Icon {...defaultProps} data-sample='Sample' />;

test('click on icon', () => {
let wrapper = mount(defaultIcon);
let wrapper = mount(clickIcon);
wrapper.find('.sap-icon--cart').simulate('click');
expect(wrapper.prop('onClick')).toBeCalledTimes(1);

Expand All @@ -22,9 +22,19 @@ describe('<Icon />', () => {
expect(wrapper.prop('onClick')).toBeUndefined;
});

test('passes aria-label prop', () => {
const element = mount(ariaLabelIcon);
expect(element.getDOMNode().getAttribute('aria-label')).toBe('Add to cart');
});

test('passes the aria-hidden prop', () => {
const element = mount(defaultIcon);
expect(element.getDOMNode().getAttribute('aria-hidden')).toBe('true');
});

describe('Prop spreading', () => {
test('should allow props to be spread to the Icon component', () => {
const element = mount(<Icon data-sample='Sample' glyph='cart' />);
const element = mount(propSpreadIcon);

expect(
element.getDOMNode().attributes['data-sample'].value
Expand All @@ -39,7 +49,7 @@ describe('<Icon />', () => {
super(props);
ref = React.createRef();
}
render = () => <Icon glyph='cart' ref={ref} />;
render = () => <Icon {...defaultProps} ref={ref} />;
}
mount(<Test />);
expect(ref.current.tagName).toEqual('SPAN');
Expand Down
6 changes: 5 additions & 1 deletion src/Select/Select.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import classnames from 'classnames';
import { FORM_MESSAGE_TYPES } from '../utils/constants';
import FormMessage from '../Forms/_FormMessage';
import FormValidationOverlay from '../Forms/_FormValidationOverlay';
import Icon from '../Icon/Icon';
import keycode from 'keycode';
import List from '../List/List';
import Popover from '../Popover/Popover';
Expand Down Expand Up @@ -135,7 +136,10 @@ const Select = React.forwardRef(({
<span aria-label={selectAriaLabel} className={textContentClassNames}>{textContent}</span>
{!readOnly &&
<span className={triggerClassNames}>
<i className='sap-icon--slim-arrow-down' />
<Icon
ariaHidden
glyph='slim-arrow-down'
isInButton />
</span>
}
</div>
Expand Down