Skip to content

Commit

Permalink
feat(Button): Remove the primary invert option for Button.
Browse files Browse the repository at this point in the history
Also improve the wording of the docs.
  • Loading branch information
ryanoglesby08 committed Aug 9, 2017
1 parent e145127 commit 9988ca5
Show file tree
Hide file tree
Showing 12 changed files with 89 additions and 39 deletions.
2 changes: 1 addition & 1 deletion config/styleguide.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ module.exports = {
name: 'Forms',
components() {
return [
// path.resolve('src/components/Button/Button.jsx'),
path.resolve('src/components/Button/Button.jsx'),
path.resolve('src/components/SelectorCounter/SelectorCounter.jsx')
]
}
Expand Down
10 changes: 9 additions & 1 deletion src/components/Button/Button.jsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
import React from 'react';
import PropTypes from 'prop-types';

import { warn } from '../../warn';

import styles from './Button.modules.scss';

const getClassName = (variant, invert) => {
if (variant === 'primary' && invert) {
warn('Button', 'Primary buttons cannot be inverted.');

return styles.primary;
}

if (invert) {
return styles[`${variant}Inverted`];
}
Expand All @@ -15,7 +23,7 @@ const safeRest = ({ style, className, ...props }) => props;

/**
*
* <span class="docs--badge green">coming soon!</span>
* <span class="docs--badge green">new!</span>
*/
const Button = ({ type, variant, invert, children, ...rest }) => (
<button {...safeRest(rest)} type={type} className={getClassName(variant, invert)}>
Expand Down
25 changes: 19 additions & 6 deletions src/components/Button/Button.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
## Minimal Usage

Provide a function in the `onClick` prop to perform an action when clicked. **Avoid using a button if navigation
Provide a function as the `onClick` prop to perform an action when clicked. **Avoid using a button if navigation
is the primary action, as a link is more appropriate.**

### Recommendations
Expand All @@ -10,7 +10,8 @@ is the primary action, as a link is more appropriate.**
* Avoid excessively long button text.
* Make sure the button text describes an action.

By default, Buttons will be displayed in the `primary` variant.
By default, Buttons will be displayed in the `primary` variant. Use primary buttons for the main action on a page or
in a form.

```
<Button>Submit</Button>
Expand All @@ -22,12 +23,24 @@ Specify the `variant` to create a button for secondary actions.
<Button variant="secondary">Find out more</Button>
```

## On images or colors
## Placing buttons on solid colors

Use the `outlined` variant when placing a button on top of a solid color other than white or on an image. Inverting the
color scheme with the `invert` attribute is another option.
Use the `secondary` `invert` button on top of the solid TELUS purple.

Edit the code block to try different combinations of `variant` and `invert`.
```
const PurpleBlock = require('./__docs__/PurpleBlock').default;
<PurpleBlock>
<Button variant="secondary" invert>Go back</Button>
</PurpleBlock>
```

## Placing buttons on images

Use the `outlined` variant when placing a button on top of an image, which will cause the image to show through. You can
also `invert` the color scheme.

Use this variant with caution. There will be accessibility issues if the color contrast of the image and the button text is too low.

```
const Hero = require('./__docs__/Hero').default;
Expand Down
13 changes: 3 additions & 10 deletions src/components/Button/Button.modules.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
@import '../../scss/settings/colours';
@import '../../scss/settings/spacing';
@import '../../scss/settings/typography';
@import '../../scss/settings/responsive-grid';
@import '../../scss/utility/responsive';

$primary-bg-color: $color-primary;
Expand Down Expand Up @@ -72,16 +73,6 @@ $text-color: $color-white;
}
}

.primaryInverted {
composes: inverted button;

color: $primary-bg-color;

&:hover {
background-color: $primary-bg-color;
}
}

.secondary {
composes: button;

Expand Down Expand Up @@ -117,6 +108,8 @@ $text-color: $color-white;
&:hover {
background-color: $text-color;
color: $secondary-bg-color;

box-shadow: none;
}
}

Expand Down
11 changes: 11 additions & 0 deletions src/components/Button/__docs__/PurpleBlock.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from 'react';
import PropTypes from 'prop-types';

import styles from './PurpleBlock.modules.scss';

const PurpleBlock = ({ children }) => <div className={styles.purpleBlock}>{children}</div>;
PurpleBlock.propTypes = {
children: PropTypes.node.isRequired
};

export default PurpleBlock;
7 changes: 7 additions & 0 deletions src/components/Button/__docs__/PurpleBlock.modules.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@import '../../../scss/settings/colours';
@import '../../../scss/settings/spacing';

.purpleBlock {
background-color: $color-secondary;
padding: $spacing-wide;
}
18 changes: 14 additions & 4 deletions src/components/Button/__tests__/Button.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@ import React from 'react';
import { shallow } from 'enzyme';
import toJson from 'enzyme-to-json';

import { warn } from '../../../warn';

import Button from '../Button';

jest.mock('../../../warn', () => (
{ warn: jest.fn() }
));

describe('Button', () => {
const doShallow = (overrides = {}) => shallow(<Button {...overrides}>Submit</Button>);

Expand Down Expand Up @@ -35,17 +41,21 @@ describe('Button', () => {
expect(button).toHaveClassName('outlined');
});

it('can be inverted', () => {
const primaryButton = doShallow({ variant: 'primary', invert: true });
expect(primaryButton).toHaveClassName('primaryInverted');

it('can be inverted for secondary and outlined variants', () => {
const secondaryButton = doShallow({ variant: 'secondary', invert: true });
expect(secondaryButton).toHaveClassName('secondaryInverted');

const outlinedButton = doShallow({ variant: 'outlined', invert: true });
expect(outlinedButton).toHaveClassName('outlinedInverted');
});

it('can not be inverted for primary variant', () => {
const button = doShallow({ variant: 'primary', invert: true });

expect(button).toHaveClassName('primary');
expect(warn).toHaveBeenCalled();
});

it('passes additional attributes to button element', () => {
const button = doShallow({ id: 'the-button', tabindex: 1 });

Expand Down
6 changes: 3 additions & 3 deletions src/components/Card/Card.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import PropTypes from 'prop-types';

import classNames from 'classnames';

import { warn } from '../../deprecate';
import { deprecate } from '../../warn';

import './Card.scss';

Expand All @@ -12,11 +12,11 @@ import './Card.scss';
*/
const Card = ({ className, children, ...rest }) => {
if (className) {
warn('Card', 'Custom CSS classes are deprecated. This component does not support custom styling.');
deprecate('Card', 'Custom CSS classes are deprecated. This component does not support custom styling.');
}

if (rest.style) {
warn('Card', 'Inline styles are deprecated. This component does not support custom styling.');
deprecate('Card', 'Inline styles are deprecated. This component does not support custom styling.');
}

return (
Expand Down
10 changes: 5 additions & 5 deletions src/components/Card/__tests__/Card.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import React from 'react';
import { shallow } from 'enzyme';
import toJson from 'enzyme-to-json';

import { warn } from '../../../deprecate';
import { deprecate } from '../../../warn';
import Card from '../Card';

jest.mock('../../../deprecate', () => (
{ warn: jest.fn() }
jest.mock('../../../warn', () => (
{ deprecate: jest.fn() }
));

describe('<Card />', () => {
Expand All @@ -27,14 +27,14 @@ describe('<Card />', () => {
const card = shallow(<Card className="some-class">Some content</Card>);

expect(card).toHaveClassName('some-class');
expect(warn).toHaveBeenCalled();
expect(deprecate).toHaveBeenCalled();
});

it('accepts but deprecates inline styles', () => {
const styles = { color: 'blue' };
const card = shallow(<Card style={styles}>Some content</Card>);

expect(card).toHaveProp('style', styles);
expect(warn).toHaveBeenCalled();
expect(deprecate).toHaveBeenCalled();
});
});
6 changes: 3 additions & 3 deletions src/components/Notification/Notification.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';

import { warn } from '../../deprecate';
import { deprecate } from '../../warn';

import './Notification.scss';

Expand All @@ -15,11 +15,11 @@ const ErrorIcon = () => (
*/
const Notification = ({ className, variant, children, ...rest }) => {
if (className) {
warn('Notification', 'Custom CSS classes are deprecated. This component does not support custom styling.');
deprecate('Notification', 'Custom CSS classes are deprecated. This component does not support custom styling.');
}

if (rest.style) {
warn('Notification', 'Inline styles are deprecated. This component does not support custom styling.');
deprecate('Notification', 'Inline styles are deprecated. This component does not support custom styling.');
}

const classes = classNames('notification', className, {
Expand Down
10 changes: 5 additions & 5 deletions src/components/Notification/__tests__/Notification.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import React from 'react';
import { shallow } from 'enzyme';
import toJson from 'enzyme-to-json';

import { warn } from '../../../deprecate';
import { deprecate } from '../../../warn';
import Notification from '../../Notification/Notification';

jest.mock('../../../deprecate', () => (
{ warn: jest.fn() }
jest.mock('../../../warn', () => (
{ deprecate: jest.fn() }
));

describe('<Notification />', () => {
Expand Down Expand Up @@ -47,14 +47,14 @@ describe('<Notification />', () => {
const notification = doShallow({ className: 'some-class' });

expect(notification).toHaveClassName('some-class');
expect(warn).toHaveBeenCalled();
expect(deprecate).toHaveBeenCalled();
});

it('accepts but deprecates inline styles', () => {
const style = { color: 'blue' };
const notification = doShallow({ style });

expect(notification).toHaveProp('style', style);
expect(warn).toHaveBeenCalled();
expect(deprecate).toHaveBeenCalled();
});
});
10 changes: 9 additions & 1 deletion src/deprecate.js → src/warn.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
/* eslint-disable import/prefer-default-export */

export const warn = (componentName, message) => {
export const deprecate = (componentName, message) => {
if (process.env.NODE_ENV === 'production') {
return;
}

console.warn(`[TDS] [Deprecate] ${componentName}: ${message}`); // eslint-disable-line no-console
};

export const warn = (componentName, message) => {
if (process.env.NODE_ENV === 'production') {
return;
}

console.warn(`[TDS] ${componentName}: ${message}`); // eslint-disable-line no-console
};

0 comments on commit 9988ca5

Please sign in to comment.