From 616e749fe21b6088c712225a3f10fa2c5aba8a78 Mon Sep 17 00:00:00 2001 From: Bernard Date: Fri, 13 Dec 2019 12:21:03 -0600 Subject: [PATCH] [WNMGDS-326] Fix react button styling (#572) * Add combined ds-c-button--transparent ds-c-button--inverse styles as equivalent to --transparent-inverse * Update Button.jsx, example and tests * Update Badge example to not use --- .../src/components/Badge/Badge.example.jsx | 8 +-- .../src/components/Button/Button.example.jsx | 23 +++---- .../core/src/components/Button/Button.jsx | 26 ++++---- .../core/src/components/Button/Button.scss | 64 ++++++++++++------- .../src/components/Button/Button.test.jsx | 46 ++++++------- 5 files changed, 92 insertions(+), 75 deletions(-) diff --git a/packages/core/src/components/Badge/Badge.example.jsx b/packages/core/src/components/Badge/Badge.example.jsx index 2a5f11b7d9..d14a13cabf 100644 --- a/packages/core/src/components/Badge/Badge.example.jsx +++ b/packages/core/src/components/Badge/Badge.example.jsx @@ -5,12 +5,8 @@ import ReactDOM from 'react-dom'; ReactDOM.render( Default badge - - Badge with variation prop - - - Badge with size prop - + Badge with `variation` prop + Badge with `size` prop , document.getElementById('js-example') ); diff --git a/packages/core/src/components/Button/Button.example.jsx b/packages/core/src/components/Button/Button.example.jsx index a9a5afab3c..75d03b38f4 100644 --- a/packages/core/src/components/Button/Button.example.jsx +++ b/packages/core/src/components/Button/Button.example.jsx @@ -1,18 +1,19 @@ +import React, { Fragment } from 'react'; import Button from './Button'; -import React from 'react'; import ReactDOM from 'react-dom'; ReactDOM.render( -
- - - + + + -
, + , document.getElementById('js-example') ); diff --git a/packages/core/src/components/Button/Button.jsx b/packages/core/src/components/Button/Button.jsx index a2144f901a..54c7404fc0 100644 --- a/packages/core/src/components/Button/Button.jsx +++ b/packages/core/src/components/Button/Button.jsx @@ -57,24 +57,26 @@ export class Button extends React.PureComponent { } classNames() { - let variationClass = this.props.variation && `ds-c-button--${this.props.variation}`; - let disabledClass = this.props.disabled && 'ds-c-button--disabled'; + const variationClass = this.props.variation && `ds-c-button--${this.props.variation}`; + const disabledClass = this.props.disabled && 'ds-c-button--disabled'; + const sizeClass = this.props.size && `ds-c-button--${this.props.size}`; + let inverseClass = this.props.inverse && 'ds-c-button--inverse'; - if (this.props.inverse) { - if (disabledClass) { - disabledClass += '-inverse'; - } else if (variationClass) { - variationClass += '-inverse'; - } else { - variationClass = 'ds-c-button--inverse'; - } + // primary/danger/success variations don't need the inverse class + if ( + this.props.variation === 'primary' || + this.props.variation === 'danger' || + this.props.variation === 'success' + ) { + inverseClass = ''; } return classNames( 'ds-c-button', disabledClass, - !disabledClass && variationClass, - this.props.size && `ds-c-button--${this.props.size}`, + variationClass, + inverseClass, + sizeClass, this.props.className ); } diff --git a/packages/core/src/components/Button/Button.scss b/packages/core/src/components/Button/Button.scss index 30887d72a1..6e359ae717 100644 --- a/packages/core/src/components/Button/Button.scss +++ b/packages/core/src/components/Button/Button.scss @@ -40,6 +40,10 @@ } } +.ds-c-button > svg { + @include inline-icon; +} + .ds-c-button--big { font-size: $h3-font-size; padding-bottom: $spacer-2; @@ -183,9 +187,9 @@ Inverse buttons */ .ds-c-button--inverse, -.ds-c-button--inverse:visited, -.ds-c-button--transparent-inverse, -.ds-c-button--transparent-inverse:visited { +.ds-c-button--inverse:visited { + background-color: $color-background-inverse; + border-color: $border-color-inverse; color: $color-base-inverse; &:focus, @@ -204,47 +208,61 @@ Inverse buttons } } -.ds-c-button--inverse, -.ds-c-button--inverse:visited { - background-color: $color-background-inverse; - border-color: $border-color-inverse; +/* +Equivalent to ".ds-c-button--disabled-inverse" +*/ +.ds-c-button--inverse:disabled, +.ds-c-button--inverse.ds-c-button--disabled { + background-color: darken($color-background-inverse, 10%); + border-color: darken($color-background-inverse, 10%); + color: $color-muted-inverse; + pointer-events: none; +} +/* +Equivalent to ".ds-c-button--transparent-inverse" +*/ +.ds-c-button--inverse.ds-c-button--transparent, +.ds-c-button--inverse.ds-c-button--transparent:visited { + border-color: $color-background-inverse; + &:disabled, - &.ds-c-button--disabled { - background-color: darken($color-background-inverse, 10%); - border-color: darken($color-background-inverse, 10%); - color: $color-muted-inverse; - pointer-events: none; - } - &:focus, &:hover, &:active, + &.ds-c-button--disabled, &.ds-c-button--focus, &.ds-c-button--hover, &.ds-c-button--active { - border-color: $color-base-inverse; + border-color: $color-background-inverse; } } +/* stylelint-disable */ .ds-c-button--transparent-inverse, .ds-c-button--transparent-inverse:visited { + @extend .ds-c-button--inverse; + border-color: $color-background-inverse; + &:disabled, - &.ds-c-button--disabled { - color: $color-muted-inverse; + &:focus, + &:hover, + &:active, + &.ds-c-button--disabled, + &.ds-c-button--focus, + &.ds-c-button--hover, + &.ds-c-button--active { + border-color: $color-background-inverse; } } -/* stylelint-disable */ -.ds-c-button--disabled-inverse { +.ds-c-button--disabled-inverse, +.ds-c-button--disabled-inverse:visited { @extend .ds-c-button--inverse; background-color: darken($color-background-inverse, 10%); border-color: darken($color-background-inverse, 10%); color: $color-muted-inverse; pointer-events: none; } -/* stylelint-enable */ -.ds-c-button > svg { - @include inline-icon; -} +/* stylelint-enable */ diff --git a/packages/core/src/components/Button/Button.test.jsx b/packages/core/src/components/Button/Button.test.jsx index 1b6e2dbf43..209de335a0 100644 --- a/packages/core/src/components/Button/Button.test.jsx +++ b/packages/core/src/components/Button/Button.test.jsx @@ -71,23 +71,22 @@ describe('Button', () => { expect(wrapper.render().text()).toBe(buttonText); }); - it('appends additional class names', () => { + it('applies additional classes', () => { const props = { className: 'foobar' }; const wrapper = shallow(); expect(wrapper.hasClass('foobar')).toBe(true); expect(wrapper.hasClass('ds-c-button')).toBe(true); }); - it('renders as a success button', () => { - const props = { variation: 'success' }; + it('applies variation classes', () => { + const props = { variation: 'danger' }; const wrapper = shallow(); expect(wrapper.hasClass('ds-c-button')).toBe(true); - expect(wrapper.hasClass('ds-c-button--success')).toBe(true); - expect(wrapper.hasClass('ds-c-button--primary')).toBe(false); + expect(wrapper.hasClass('ds-c-button--danger')).toBe(true); }); - it('renders as a small button', () => { + it('applies size classes', () => { const props = { size: 'small' }; const wrapper = shallow(); @@ -95,46 +94,47 @@ describe('Button', () => { expect(wrapper.hasClass('ds-c-button--small')).toBe(true); }); - it('overrides variation class when disabled', () => { - const props = { - disabled: true, - variation: 'primary' - }; + it('applies disabled class', () => { + const props = { disabled: true }; const wrapper = shallow(); + expect(wrapper.hasClass('ds-c-button')).toBe(true); expect(wrapper.hasClass('ds-c-button--disabled')).toBe(true); - expect(wrapper.hasClass('ds-c-button--primary')).toBe(false); }); - it('applies inverse suffix to variation class', () => { + it('applies disabled, inverse, and variation classes together', () => { const props = { + disabled: true, inverse: true, variation: 'transparent' }; const wrapper = shallow(); - expect(wrapper.hasClass('ds-c-button--transparent-inverse')).toBe(true); - expect(wrapper.hasClass('ds-c-button--transparent')).toBe(false); + expect(wrapper.hasClass('ds-c-button--transparent')).toBe(true); + expect(wrapper.hasClass('ds-c-button--inverse')).toBe(true); + expect(wrapper.hasClass('ds-c-button--disabled')).toBe(true); expect(wrapper.hasClass('ds-c-button')).toBe(true); - expect(wrapper.hasClass('ds-c-button--inverse')).toBe(false); }); - it('applies inverse suffix to disabled class', () => { + it('doesnt apply inverse to primary/danger/success variations', () => { const props = { inverse: true, - disabled: true + variation: 'primary' }; const wrapper = shallow(); - expect(wrapper.hasClass('ds-c-button--disabled-inverse')).toBe(true); - expect(wrapper.hasClass('ds-c-button')).toBe(true); + expect(wrapper.hasClass('ds-c-button--inverse')).toBe(false); + expect(wrapper.hasClass('ds-c-button--primary')).toBe(true); }); - it('applies inverse suffix to default button class', () => { - const props = { inverse: true }; + it('applies inverse to default/transparent variations', () => { + const props = { + inverse: true, + variation: 'transparent' + }; const wrapper = shallow(); expect(wrapper.hasClass('ds-c-button--inverse')).toBe(true); - expect(wrapper.hasClass('ds-c-button')).toBe(true); + expect(wrapper.hasClass('ds-c-button--transparent')).toBe(true); }); });