Crayons: buttons followup (#15720)

* init

* whoops

* components update

* components update

* classes

* variables

* focus

* test

* spec

* test

* adjust colors and variables

* buttons colors

* Update app/javascript/crayons/CTAs/CTA.jsx

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* proptype

* remove duplicated tests now in Cypress, enhance Cypress nav menu tests

* premove unneeded viewport in test

* remove focus check for now

* classnames

* Update app/javascript/crayons/CTAs/__stories__/CTAs.mdx

Co-authored-by: Nick Taylor <nick@iamdeveloper.com>

* Update app/javascript/crayons/CTAs/__stories__/CTAs.mdx

Co-authored-by: Nick Taylor <nick@iamdeveloper.com>

* better contrast

* classname

* add secondary button

* variants + docs

* docs updates

* radius-full

* variants oneof

* the?

* default values + extra stories

* Apply suggestions from code review

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

app/assets/stylesheets/components/buttons.scss

@@ -513,6 +513,13 @@ label[class*='crayons-btn']:focus-within // label for input[type="file"] made to
   }
 }
 
+.c-btn--secondary {
+  --bg: var(--btn-secondary-bg);
+  --bg-hover: var(--btn-secondary-bg-hover);
+  --color: var(--btn-secondary-color);
+  --color-hover: var(--btn-secondary-color-hover);
+}
+
 .c-btn--icon-alone {
   padding: var(--su-2);
 }

app/assets/stylesheets/components/links.scss

@@ -92,7 +92,13 @@
 
 .c-link--icon-left {
   .c-link__icon {
-    margin-right: var(--su-2);
-    margin-left: calc(var(--su-1) * -1);
+    margin-right: var(--su-1);
+  }
+
+  &.c-link--block {
+    .c-link__icon {
+      margin-right: var(--su-2);
+      margin-left: calc(var(--su-1) * -1);
+    }
   }
 }

app/assets/stylesheets/config/_colors.scss

@@ -257,19 +257,25 @@
   --btn-current-bg: var(--base-20);
   --btn-current-color: var(--base-100);
 
-  // Buttons: Default Destructive
-  --btn-destructive-bg: transparent;
-  --btn-destructive-bg-hover: var(--accent-danger-a10);
-  --btn-destructive-color: var(--accent-danger);
-  --btn-destructive-color-hover: var(--accent-danger-darker);
-
   // Buttons: Primary
   --btn-primary-bg: var(--accent-brand);
   --btn-primary-bg-hover: var(--accent-brand-darker);
   --btn-primary-color: var(--base-inverted);
   --btn-primary-color-hover: var(--base-inverted);
 
-  // Buttons: Primary Destructive
+  // Buttons: Secondary
+  --btn-secondary-bg: var(--accent-brand-a10);
+  --btn-secondary-bg-hover: var(--accent-brand);
+  --btn-secondary-color: var(--accent-brand);
+  --btn-secondary-color-hover: var(--base-inverted);
+
+  // Buttons: Destructive Default
+  --btn-destructive-bg: transparent;
+  --btn-destructive-bg-hover: var(--accent-danger-a10);
+  --btn-destructive-color: var(--accent-danger);
+  --btn-destructive-color-hover: var(--accent-danger-darker);
+
+  // Buttons: Destructive Primary
   --btn-primary-destructive-bg: var(--accent-danger);
   --btn-primary-destructive-bg-hover: var(--accent-danger-darker);
   --btn-primary-destructive-color: var(--base-inverted);

app/javascript/crayons/Buttons/Button.jsx

@@ -8,7 +8,7 @@ import { Icon } from '@crayons';
 export const ButtonNew = (props) => {
   const {
     children,
-    primary,
+    variant = 'default',
     icon,
     rounded,
     destructive,
@@ -30,8 +30,8 @@ export const ButtonNew = (props) => {
   };
 
   const classes = classNames('c-btn', {
-    'c-btn--primary': primary,
-    'c-btn--destructive': destructive,
+    [`c-btn--${variant}`]: variant && variant !== 'default',
+    'c-btn--destructive': destructive && variant !== 'secondary',
     'c-btn--icon-left': icon && children,
     'c-btn--icon-alone': icon && !children,
     'crayons-tooltip__activator': tooltip,
@@ -73,7 +73,7 @@ ButtonNew.displayName = 'ButtonNew';
 
 ButtonNew.propTypes = {
   children: defaultChildrenPropTypes,
-  primary: PropTypes.bool,
+  variant: PropTypes.oneOf(['default', 'primary', 'secondary']),
   rounded: PropTypes.bool,
   destructive: PropTypes.bool,
   type: PropTypes.oneOf(['button', 'submit']),

app/javascript/crayons/Buttons/__stories__/Buttons.mdx

@@ -14,35 +14,50 @@ That is: `<CTA />` or `<Link />` should be used where navigation is elicited, an
 For example, a 'Submit' control on 'Sign in' view (form) would suit a `<Button />` because its job is to submit the form. 
 However, 'Register' call-to-action—taking the user to a different form, in a different page or screen—would benefit from `<CTA />` or `<Link />` semantics.
 
-Do not use `<Button />s` as navigational elements. Instead, use Links (or CTAs) when the desired action is to take the user to a new page.
+Do not use `<Button />s` as navigational elements. Instead, use `<Link />s` (or `<CTA />s`) when the desired action is to take the user to a new page.
 
 ## Variants
 
-### Primary
-
-Primary button has special look: high contrast (against rest of the view/component) and it's filled with accent color. 
-
-Ideally there should be only one Primary button per entire component, or sometimes even per entire view.
-Primary buttons should be used for the principal call to action on the page.
-
 ### Default
 
-Default variant is less prominent, and therefore can be used for less prominent actions.
-Default buttons are visually light (greyish) and you can have as many of them as you want (within common sense :D). 
-They can (but don't have to) be used in conjunction with a Primary button. In that case, default variant should be used for any remaining actions.
+The _Default_ variant is not very prominent, and therefore can be used for less prominent actions.
+The _Default_ buttons are visually light (greyish) and you can have as many of them as you want (within common sense :D). 
+They can (but don't have to) be used in conjunction with a _Primary_ button and less often with _Secondary_ buttons (use your common sense again or ping your designer).
+When combined with the _Primary_ or the _Secondary_, the _Default_ variant should be used for any remaining actions.
+
+Whenever used, make sure it has sufficient color contrast against the background it's used on.
+
+### Primary
+
+The _Primary_ variant has a special look: high contrast (against rest of the view/component) and it's filled with accent color. 
+
+Ideally there should be only one _Primary_ button per entire component, or sometimes even per entire view.
+Primary buttons should be used for the principal action on the page.
+
+### Secondary
+
+The _Secondary_ variant is visually in between the _Primary_ and the _Default_ buttons. It's not as loud as the _Primary_ button but definitely stands out more than the _Default_ button.
+It's especially useful when you want your button to be more distinguishable from its surroundings without making it super loud (like _Primary_). 
+A good example would be a component with lots of text. The _Default_ button could be potentially missed there (as it uses similar colors as regular texts).
+
+Whenever you have doubts between using the _Secondary_ vs the _Default_ variants, the rule of thumb should be:
+
+> Use the _Default_ variant by default :D unless it's not distinguishable enough from its surroundings.
+
+When combined with the _Primary_ button, the _Secondary_ variant should be used for any remaining or alternative actions.
 
 ## Configuration
 
 ### Destructive actions
 
 For actions that could have destructive effects on the user’s data (for example, *delete* or *remove*) we have a special style.
-This style can be applied by using `destructive` prop and it will work for both: primary and default variants. Take care when using the default variant to ensure that the red text has sufficient color contrast against the background where you are using it.
+This style can be applied by using `destructive` prop and it will work with two variants: _Primary_ and _Default_ (as it doesn't really make sense to have _Secondary_ variant for destructive buttons).
 
 ### Icons
 
-Icon can be placed in front of a label to both clarify an action and call attention to a button. 
+Icon can be placed in front of a label to both clarify an action but also to make button stand out more.
 By passing an object in `icon` prop, there will be automatically included `<Icon />` component.
-By default, the icon itself won't be customizable. Icons within buttons are hidden from assistive technology, so make sure your button has descriptive enough text, or is otherwise well-labelled (e.g. with `aria-label`).
+By default, the icon itself within `<Button />` is **not** customizable. Icons within buttons are hidden from assistive technology, so make sure your button has descriptive enough text, or is otherwise well-labelled (e.g. with `aria-label`).
 
 #### Icon-only
 

app/javascript/crayons/Buttons/__stories__/Buttons.stories.jsx

@@ -12,11 +12,19 @@ export default {
     },
   },
   argTypes: {
-    primary: {
+    variant: {
+      control: {
+        type: 'select',
+        options: {
+          default: undefined,
+          primary: 'primary',
+          secondary: 'secondary',
+        },
+      },
       description:
-        'This prop defines whether or not your button will have *primary* style (in practice: high contrast, filled with accent color). Keep in mind ideally there should be only one primary button per entire component, or sometimes even per entire view.',
+        'There are three available variants (styles) to pick from: _default_, _primary_ and _secondary_. Please refer to the documentation to better understand the differences in usage.',
       table: {
-        defaultValue: { summary: false },
+        defaultValue: { summary: 'default' },
       },
     },
     rounded: {
@@ -28,7 +36,7 @@ export default {
     },
     destructive: {
       description:
-        'For various destructive actions we can have special styling for button which will add red-ish coloring.',
+        'For various destructive actions we can have special styling for button which will add red-ish coloring. Keep in mind we only have two variants available for destructive button: _default_ and _primary_',
       table: {
         defaultValue: { summary: false },
       },
@@ -56,7 +64,6 @@ export default {
 
 export const Default = (args) => <Button {...args} />;
 Default.args = {
-  primary: false,
   destructive: false,
   children: 'Button label',
   tooltip: undefined,
@@ -66,7 +73,25 @@ Default.args = {
 export const Primary = (args) => <Button {...args} />;
 Primary.args = {
   ...Default.args,
-  primary: true,
+  variant: 'primary',
+};
+
+export const Secondary = (args) => <Button {...args} />;
+Secondary.args = {
+  ...Default.args,
+  variant: 'secondary',
+};
+
+export const Destructive = (args) => <Button {...args} />;
+Destructive.args = {
+  ...Default.args,
+  destructive: true,
+};
+
+export const WithTooltip = (args) => <Button {...args} />;
+WithTooltip.args = {
+  ...Default.args,
+  tooltip: 'Hello world',
 };
 
 export const WithIcon = (args) => <Button {...args} />;
@@ -74,3 +99,11 @@ WithIcon.args = {
   ...Default.args,
   icon: CogIcon,
 };
+
+export const IconOnly = (args) => <Button {...args} />;
+IconOnly.args = {
+  ...Default.args,
+  icon: CogIcon,
+  tooltip: 'Button label',
+  children: undefined,
+};

app/javascript/crayons/Buttons/__tests__/Buttons.test.jsx

@@ -13,9 +13,33 @@ describe('<Button />', () => {
     expect(results).toHaveNoViolations();
   });
 
+  it('has no accessibility errors in primary variant', async () => {
+    const { container } = render(
+      <Button variant="primary">Hello world!</Button>,
+    );
+    const results = await axe(container);
+
+    expect(results).toHaveNoViolations();
+  });
+
+  it('has no accessibility errors in secondary variant', async () => {
+    const { container } = render(
+      <Button variant="secondary">Hello world!</Button>,
+    );
+    const results = await axe(container);
+
+    expect(results).toHaveNoViolations();
+  });
+
   it('has no accessibility errors when props provided', async () => {
     const { container } = render(
-      <Button primary rounded destructive icon={CogIcon} tooltip="tooltip">
+      <Button
+        variant="primary"
+        rounded
+        destructive
+        icon={CogIcon}
+        tooltip="tooltip"
+      >
         Hello world!
       </Button>,
     );
@@ -30,7 +54,16 @@ describe('<Button />', () => {
   });
 
   it('renders a primary button', () => {
-    const { container } = render(<Button primary>Hello world!</Button>);
+    const { container } = render(
+      <Button variant="primary">Hello world!</Button>,
+    );
+    expect(container.innerHTML).toMatchSnapshot();
+  });
+
+  it('renders a secondary button', () => {
+    const { container } = render(
+      <Button variant="secondary">Hello world!</Button>,
+    );
     expect(container.innerHTML).toMatchSnapshot();
   });
 

app/javascript/crayons/Buttons/__tests__/__snapshots__/Buttons.test.jsx.snap

@@ -8,6 +8,8 @@ exports[`<Button /> renders a primary button 1`] = `"<button type=\\"button\\" c
 
 exports[`<Button /> renders a rounded button 1`] = `"<button type=\\"button\\" class=\\"c-btn radius-full\\">Hello world!</button>"`;
 
+exports[`<Button /> renders a secondary button 1`] = `"<button type=\\"button\\" class=\\"c-btn c-btn--secondary\\">Hello world!</button>"`;
+
 exports[`<Button /> renders with a tooltip 1`] = `"<button type=\\"button\\" class=\\"c-btn crayons-tooltip__activator\\">Hello world!<span data-testid=\\"tooltip\\" class=\\"crayons-tooltip__content\\">tooltip text</span></button>"`;
 
 exports[`<Button /> renders with additional classnames 1`] = `"<button type=\\"button\\" class=\\"c-btn one two three\\">Hello world!</button>"`;

app/javascript/crayons/CTAs/CTA.jsx

@@ -8,14 +8,14 @@ export const CTA = (props) => {
   const {
     children,
     href = '#',
-    variant,
+    variant = 'default',
     icon,
     className,
     ...otherProps
   } = props;
 
   const classes = classNames('c-cta', {
-    [`c-cta--${variant}`]: variant,
+    [`c-cta--${variant}`]: variant && variant !== 'default',
     'c-cta--icon-left': icon && children,
     [className]: className,
   });
@@ -38,7 +38,7 @@ export const CTA = (props) => {
 CTA.displayName = 'CTA';
 
 CTA.propTypes = {
-  variant: PropTypes.oneOf([undefined, 'branded']),
+  variant: PropTypes.oneOf(['default', 'branded']),
   rounded: PropTypes.bool,
   href: PropTypes.string.isRequired,
   className: PropTypes.string,

app/javascript/crayons/CTAs/__stories__/CTAs.mdx

@@ -16,12 +16,11 @@ Do not use `<Button />s` as navigational elements. Instead, use Links (or CTAs)
 A CTA renders an anchor element, therefore standard link markup must be used,
 including a `href` attribute, otherwise, it is not focusable by the keyboard.
 
-
 ### Icons
 
-Icon can be placed in front of a label to both clarify an action and call attention to a CTA.
+Icon can be placed in front of a label to both clarify an action but also to make CTA stand out more.
 By passing an object in `icon` prop, there will be automatically included `<Icon />` component.
-By default, the icon itself won't be customizable. Icons within CTAs are hidden from assistive technology, so make sure your button has descriptive enough text, or is otherwise well-labelled (e.g. with `aria-label`).
+By default, the icon itself within `<CTA />` is **not** customizable. Icons within CTAs are hidden from assistive technology, so make sure your CTA has descriptive enough text, or is otherwise well-labelled (e.g. with `aria-label`).
 
 #### Icon-only
 

app/javascript/crayons/CTAs/__stories__/CTAs.stories.jsx

@@ -40,8 +40,14 @@ Branded.args = {
   variant: 'branded',
 };
 
-export const WithIcon = (args) => <CTA {...args} />;
-WithIcon.args = {
+export const DefaultWithIcon = (args) => <CTA {...args} />;
+DefaultWithIcon.args = {
   ...Default.args,
   icon: CogIcon,
 };
+
+export const BrandedWithIcon = (args) => <CTA {...args} />;
+BrandedWithIcon.args = {
+  ...Branded.args,
+  icon: CogIcon,
+};

app/javascript/crayons/Links/Link.jsx

@@ -8,7 +8,7 @@ export const Link = (props) => {
   const {
     children,
     href = '#',
-    variant,
+    variant = 'default',
     block,
     icon,
     rounded,
@@ -17,7 +17,7 @@ export const Link = (props) => {
   } = props;
 
   const classes = classNames('c-link', {
-    [`c-link--${variant}`]: variant,
+    [`c-link--${variant}`]: variant && variant !== 'default',
     'c-link--icon-left': icon && children,
     'c-link--icon-alone': icon && !children,
     'c-link--block': block,
@@ -43,7 +43,7 @@ export const Link = (props) => {
 Link.displayName = 'Link';
 
 Link.propTypes = {
-  variant: PropTypes.oneOf([undefined, 'branded']),
+  variant: PropTypes.oneOf(['default', 'branded']),
   block: PropTypes.bool,
   rounded: PropTypes.bool,
   href: PropTypes.string.isRequired,

app/javascript/crayons/Links/__stories__/Links.mdx

@@ -27,10 +27,9 @@ It is recommended to always use `branded` variant for inline links, especially w
 
 ### Icons
 
-Icons are only supported in *block* links (`block`).
-Icon can be placed in front of a label to both clarify an action and call attention to a link.
-By passing an object in `src` prop, there will be automatically included `<Icon />` component.
-By default, the icon itself won't be customizable.
+Icon can be placed in front of a label to both clarify an action but also to make link stand out more.
+By passing an object in `icon` prop, there will be automatically included `<Icon />` component.
+By default, the icon itself within `<Link />` is **not** customizable. Icons within links are hidden from assistive technologies, so make sure your link has descriptive enough text, or is otherwise well-labelled (e.g. with `aria-label`).
 
 #### Icon-only
 

app/javascript/crayons/Links/__stories__/Links.stories.jsx

@@ -43,7 +43,7 @@ export default {
     icon: {
       control: false,
       description:
-        'Icons are only supported in *block* links (`block`). Even though the generated JSX code in this component will have an empty function as prop value (`<Link block icon={() => {}}>`), the actual value should be an icon object imported from `@images`. Read more about icons in their dedicated Storybook page.',
+        'Even though the generated JSX code in this component will have an empty function as prop value (`<Link icon={() => {}}>`), the actual value should be an icon object imported from `@images`. Read more about icons in their dedicated Storybook page.',
       table: {
         defaultValue: { summary: 'CogIcon' },
       },
@@ -53,12 +53,23 @@ export default {
 
 export const Inline = (args) => <Link {...args} />;
 Inline.args = {
-  variant: 'branded',
   block: false,
   rounded: false,
   children: 'Inline link',
 };
 
+export const InlineBranded = (args) => <Link {...args} />;
+InlineBranded.args = {
+  ...Inline.args,
+  variant: 'branded',
+};
+
+export const InlineWithIcon = (args) => <Link block {...args} />;
+InlineWithIcon.args = {
+  ...Inline.args,
+  icon: CogIcon,
+};
+
 export const Block = (args) => <Link {...args} />;
 Block.args = {
   ...Inline.args,
@@ -67,8 +78,14 @@ Block.args = {
   children: 'Block link',
 };
 
-export const WithIcon = (args) => <Link block {...args} />;
-WithIcon.args = {
+export const BlockBranded = (args) => <Link {...args} />;
+BlockBranded.args = {
+  ...Block.args,
+  variant: 'branded',
+};
+
+export const BlockWithIcon = (args) => <Link block {...args} />;
+BlockWithIcon.args = {
   ...Block.args,
   icon: CogIcon,
 };