Refactor SASS for sizes, variants, clarity

* Break out most private mixins into `_base`
* Provide `IconButton`, `LabeledButton`, `LinkButton` as public mixins
* Establish colors for all variants in `_config`
* Extend mixins to support sizes and more variants
* Update `TopBar` and `SearchInput`, as custom styling no longer
  necessary; remove those custom styles

src/sidebar/components/SearchInput.js

@@ -80,9 +80,9 @@ export default function SearchInput({ alwaysExpanded, query, onSearch }) {
       {!isLoading && (
         <div className="SearchInput__button-container">
           <IconButton
-            className="CompactIconButton"
             icon="search"
             onClick={() => input.current.focus()}
+            size="small"
             title="Search annotations"
           />
         </div>

src/sidebar/components/TopBar.js

@@ -5,9 +5,8 @@ import isThirdPartyService from '../helpers/is-third-party-service';
 import { withServices } from '../service-context';
 import { applyTheme } from '../helpers/theme';
 
-import { IconButton } from '../../shared/components/buttons';
+import { IconButton, LinkButton } from '../../shared/components/buttons';
 
-import Button from './Button';
 import GroupList from './GroupList';
 import SearchInput from './SearchInput';
 import SortMenu from './SortMenu';
@@ -83,20 +82,24 @@ function TopBar({
         <span className="TopBar__login-links">⋯</span>
       )}
       {auth.status === 'logged-out' && (
-        <span className="TopBar__login-links">
-          <Button
-            className="TopBar__login-button"
-            buttonText="Sign up"
+        <span className="TopBar__login-links u-font--large u-horizontal-rhythm">
+          <LinkButton
+            className="InlineLinkButton"
             onClick={onSignUp}
             style={loginLinkStyle}
-          />{' '}
-          /
-          <Button
-            className="TopBar__login-button"
-            buttonText="Log in"
+            variant="primary"
+          >
+            Sign up
+          </LinkButton>
+          <div>/</div>
+          <LinkButton
+            className="InlineLinkButton"
             onClick={onLogin}
             style={loginLinkStyle}
-          />
+            variant="primary"
+          >
+            Log in
+          </LinkButton>
         </span>
       )}
       {auth.status === 'logged-in' && (
@@ -113,10 +116,10 @@ function TopBar({
           <StreamSearchInput />
           <div className="u-stretch" />
           <IconButton
-            className="CompactIconButton"
             icon="help"
             expanded={isHelpPanelOpen}
             onClick={requestHelp}
+            size="small"
             title="Help"
           />
           {loginControl}
@@ -129,9 +132,9 @@ function TopBar({
           <div className="u-stretch" />
           {pendingUpdateCount > 0 && (
             <IconButton
-              className="CompactIconButton"
               icon="refresh"
               onClick={applyPendingUpdates}
+              size="small"
               variant="primary"
               title={`Show ${pendingUpdateCount} new/updated ${
                 pendingUpdateCount === 1 ? 'annotation' : 'annotations'
@@ -145,18 +148,18 @@ function TopBar({
           <SortMenu />
           {showSharePageButton && (
             <IconButton
-              className="CompactIconButton"
               icon="share"
               expanded={isAnnotationsPanelOpen}
               onClick={toggleSharePanel}
+              size="small"
               title="Share annotations on this page"
             />
           )}
           <IconButton
-            className="CompactIconButton"
             icon="help"
             expanded={isHelpPanelOpen}
             onClick={requestHelp}
+            size="small"
             title="Help"
           />
           {loginControl}

src/sidebar/components/test/TopBar-test.js

@@ -147,13 +147,10 @@ describe('TopBar', () => {
         onSignUp,
       });
       const loginText = getLoginText(wrapper);
-      const loginButtons = loginText.find('Button');
+      const loginButtons = loginText.find('LinkButton');
       assert.equal(loginButtons.length, 2);
 
-      assert.equal(loginButtons.at(0).props().buttonText, 'Sign up');
       assert.equal(loginButtons.at(0).props().onClick, onSignUp);
-
-      assert.equal(loginButtons.at(1).props().buttonText, 'Log in');
       assert.equal(loginButtons.at(1).props().onClick, onLogin);
     });
 

src/styles/shared/components/buttons/_base.scss

+@use "sass:map";
+
+@use "@hypothesis/frontend-shared/styles/mixins/focus";
+
+// Set colors for a button
+@mixin _colors($colormap) {
+  color: map.get($colormap, 'foreground');
+  background-color: map.get($colormap, 'background');
+
+  &:disabled {
+    color: map.get($colormap, 'disabled-foreground');
+  }
+}
+
+// Set hover colors and transition for a button
+@mixin _hover-state($colormap) {
+  &:hover:not([disabled]) {
+    color: map.get($colormap, 'hover-foreground');
+    background-color: map.get($colormap, 'hover-background');
+  }
+  transition: color 0.2s ease-out, background-color 0.2s ease-out,
+    opacity 0.2s ease-out;
+}
+
+// Set active state colors for a button
+@mixin _active-state($colormap) {
+  &[aria-expanded='true'],
+  &[aria-pressed='true'] {
+    color: map.get($colormap, 'active-foreground');
+    @if map.get($colormap, 'active-background') {
+      background-color: map.get($colormap, 'active-background');
+    }
+
+    &:hover:not([disabled]) {
+      color: map.get($colormap, 'hover-foreground');
+    }
+
+    &:focus:not([disabled]) {
+      color: map.get($colormap, 'active-foreground');
+    }
+  }
+}
+
+// Variant mixin: may be be used by variants (BEM modifier classes)
+@mixin button--variant($options) {
+  @include _colors(map.get($options, 'colormap'));
+  @if map.get($options, 'withStates') {
+    @include _active-state(map.get($options, 'colormap'));
+    @include _hover-state(map.get($options, 'colormap'));
+  }
+  @content;
+}
+
+// Base mixin for buttons.
+@mixin button($options) {
+  // Reset browser defaults
+  @include focus.outline-on-keyboard-focus;
+  padding: 0;
+  margin: 0;
+  background-color: transparent;
+  border-style: none;
+
+  @include _colors(map.get($options, 'colormap'));
+
+  @if map.get($options, 'withStates') {
+    @include _active-state(map.get($options, 'colormap'));
+    @include _hover-state(map.get($options, 'colormap'));
+  }
+
+  border-radius: map.get($options, 'border-radius');
+  border: none;
+  padding: 0.5em;
+
+  &--small {
+    padding: 0.25em;
+  }
+
+  &--large {
+    padding: 0.75em;
+  }
+
+  font-size: 1em;
+  font-weight: 700;
+  white-space: nowrap; // Keep multi-word button labels from wrapping
+
+  @if map.get($options, 'inline') {
+    display: inline;
+  } @else {
+    display: flex;
+    justify-content: center;
+    align-items: center;
+  }
+
+  @if map.get($options, 'withLayout') {
+    &--icon-left svg {
+      margin-right: map.get($options, 'margin');
+    }
+
+    &--icon-right svg {
+      margin-left: map.get($options, 'margin');
+    }
+    // When a button has "layout", that indicates it has some textual content:
+    // Size text to the contextual 1em, and adjust the icon to look balanced.
+    // H frontend app buttons tend to apply an icon:text ratio of ~1.25:1
+    svg {
+      width: 1.25em;
+      height: 1.25em;
+    }
+  } @else {
+    // In the case where an icon is the only content in a <button> element,
+    // size the icon based on contextual font-size. i.e. the icon IS
+    // the content
+    svg {
+      width: 1em;
+      height: 1em;
+    }
+  }
+}

src/styles/shared/components/buttons/_config.scss

@@ -7,6 +7,7 @@ $touch-target-size: var.$touch-target-size !default;
 $color-g1: var.$color-grey-1 !default;
 $color-g2: var.$color-grey-2 !default;
 $color-g3: var.$color-grey-3 !default;
+$color-g4: var.$color-grey-4 !default;
 $color-g6: var.$color-grey-6 !default;
 $color-g7: var.$color-grey-7 !default;
 $color-gsemi: var.$color-grey-semi !default;
@@ -14,62 +15,96 @@ $color-gmid: var.$color-grey-mid !default;
 $color-brand: var.$color-brand !default;
 $color-link-hover: var.$color-link-hover !default;
 
-// The following SASS maps define color sets for each type of button
-
-// Default colors for buttons
-$colors: (
+// Colors for labeled buttons
+$LabeledButton-colors: (
   'foreground': $color-gmid,
   'background': $color-g1,
   'hover-foreground': $color-g7,
   'hover-background': $color-g2,
   'active-foreground': $color-g7,
-  'active-background': $color-g1,
-  'border': transparent,
   'disabled-foreground': $color-gmid,
 );
 
-// Icon-only buttons have a transparent background by default, and a more
-// visible active (pressed) state
-$IconButton-colors: map.merge(
-  $colors,
-  (
-    'background': transparent,
-    'hover-background': transparent,
-    'active-foreground': $color-brand,
-    'active-background': transparent,
-  )
+// Variant currently unused
+$LabeledButton-colors--light: $LabeledButton-colors;
+
+$LabeledButton-colors--primary: (
+  'foreground': $color-g1,
+  'background': $color-gmid,
+  'hover-foreground': $color-g1,
+  'hover-background': $color-g6,
+  'active-foreground': $color-g1,
+  'disabled-foreground': $color-g4,
+);
+
+$LabeledButton-colors--dark: (
+  'foreground': $color-gmid,
+  'background': transparent,
+  'hover-foreground': $color-g7,
+  'hover-background': $color-g3,
+  'active-foreground': $color-gmid,
+  'active-background': $color-g3,
+  'disabled-foreground': $color-gmid,
+);
+
+// Colors for icon-only buttons
+$IconButton-colors: (
+  'foreground': $color-gmid,
+  'background': transparent,
+  'hover-foreground': $color-g7,
+  'hover-background': transparent,
+  'active-foreground': $color-brand,
+  'disabled-foreground': $color-gmid,
+);
+
+$IconButton-colors--light: (
+  'foreground': $color-gsemi,
+  'background': transparent,
+  'hover-foreground': $color-g6,
+  'hover-background': transparent,
+  'active-foreground': $color-gsemi,
+  'disabled-foreground': $color-gsemi,
+);
+
+$IconButton-colors--primary: (
+  'foreground': $color-brand,
+  'background': transparent,
+  'hover-foreground': $color-brand,
+  'hover-background': transparent,
+  'active-foreground': $color-brand,
+  'disabled-foreground': $color-gmid,
 );
 
-$IconButton-colors--primary: map.merge(
-  $IconButton-colors,
-  (
-    'foreground': $color-brand,
-    'hover-foreground': $color-brand,
-    'active-foreground': $color-brand,
-  )
+// Variant currently unused
+$IconButton-colors--dark: $IconButton-colors;
+
+// Colors for buttons styled as "links"
+$LinkButton-colors: (
+  'foreground': $color-gmid,
+  'background': transparent,
+  'hover-foreground': $color-link-hover,
+  'hover-background': transparent,
+  'active-foreground': $color-g7,
+  'disabled-foreground': $color-gmid,
 );
 
-$LabeledButton-colors: $colors;
+// Variant currently unused
+$LinkButton-colors--light: $LinkButton-colors;
 
-// This set of colors is light text on dark background
-$LabeledButton-colors--primary: map.merge(
-  $colors,
-  (
-    'foreground': $color-g1,
-    'background': $color-gmid,
-    'hover-foreground': $color-g1,
-    'hover-background': $color-g6,
-    'disabled-foreground': $color-gsemi,
-  )
+$LinkButton-colors--primary: (
+  'foreground': $color-brand,
+  'background': transparent,
+  'hover-foreground': $color-brand,
+  'hover-background': transparent,
+  'active-foreground': $color-brand,
+  'disabled-foreground': $color-gmid,
 );
 
-// This set of colors is for buttons styled as links on a white background
-$LinkButton-colors: map.merge(
-  $colors,
-  (
-    'background': transparent,
-    'hover-background': transparent,
-    'active-background': transparent,
-    'hover-foreground': $color-link-hover,
-  )
+$LinkButton-colors--dark: (
+  'foreground': $color-g7,
+  'background': transparent,
+  'hover-foreground': $color-link-hover,
+  'hover-background': transparent,
+  'active-foreground': $color-g7,
+  'disabled-foreground': $color-gmid,
 );

src/styles/shared/components/buttons/_index.scss

-@use "sass:map";
-
-@use "../../variables" as var;
-
-@use './_config' as *; // Bring variables into scope
 @use './mixins';
 
-// A button with only an icon and no label/text
-.IconButton {
-  @include mixins.Button(
-    $options: (
-      'colormap': $IconButton-colors,
-    )
-  ) {
-    @media (pointer: coarse) {
-      min-width: $touch-target-size;
-      min-height: $touch-target-size;
-    }
-  }
-  &--primary {
-    @include mixins.Button--variant(
-      $options: (
-        'colormap': $IconButton-colors--primary,
-      )
-    );
-  }
-}
-
 // A button with text, and optionally an icon
 .LabeledButton {
-  @include mixins.Button(
-    $options: (
-      'colormap': $LabeledButton-colors,
-      'withLayout': true,
-    )
-  );
-  &--primary {
-    @include mixins.Button--variant(
-      $options: (
-        'colormap': $LabeledButton-colors--primary,
-      )
-    );
-  }
+  @include mixins.LabeledButton;
+}
+
+// A button with only an icon and no label/text
+.IconButton {
+  @include mixins.IconButton;
 }
 
 // A button styled to appear as a link, with underline on hover
 .LinkButton {
-  @include mixins.Button(
-    $options: (
-      'colormap': $LinkButton-colors,
-      'withStates': false,
-    )
-  ) {
-    // Override base font-weight
-    font-weight: 400;
-  }
-  // No active states, but we do want a hover state
-  @include mixins.hover-state($LinkButton-colors) {
-    text-decoration: underline;
-  }
-  // Remove horizontal padding to allow button to appear flush-left or -right
-  padding: 0.5em 0;
+  @include mixins.LinkButton;
 }

src/styles/shared/components/buttons/mixins.scss

@@ -2,174 +2,141 @@
 
 @use "@hypothesis/frontend-shared/styles/mixins/focus";
 
-@use './_config' as *;
-
-// Reset browser native styles. This can be used in conjunction with other
-// styles via `base` or standalone for starting a custom button's styles
-// from relative scratch
-@mixin reset {
-  @include focus.outline-on-keyboard-focus;
-  // Reset native styles
-  padding: 0;
-  margin: 0;
-  background-color: transparent;
-  border-style: none;
-}
+@use './_config' as c;
+@use './_base' as base;
 
-// Set colors for a button
-@mixin _colors($colormap) {
-  color: map.get($colormap, 'foreground');
-  background-color: map.get($colormap, 'background');
+// Allow access to the configuration values to users of this module (esp. colors)
+@forward './_config';
 
-  &:disabled {
-    color: map.get($colormap, 'disabled-foreground');
-  }
-}
+// Base mixin for <button> elements
+@mixin Button($options: ()) {
+  $defaultOptions: (
+    // What colors should be used for this button's styling?
+    'colormap': c.$LabeledButton-colors,
+    // And for its variants...needed if `withVariants` is true (true is default)
+    'colormap--primary': c.$LabeledButton-colors--primary,
+    'colormap--dark': c.$LabeledButton-colors--dark,
+    'colormap--light': c.$LabeledButton-colors--light,
+    // Should this button apply an inline layout? (default is flex)
+    'inline': false,
+    // Should styling be added for "active" and "hover" states for this <button>?
+    'withStates': true,
+    // Should styling be added to support the layout of multiple sub-elements
+    // of this <button>? Not needed if <button> contains only one child.
+    'withLayout': false,
+    // Provide styling for light, primary and dark variants? If this is true,
+    // make sure all variant colormaps are provided
+    'withVariants': true,
+    // Internal margin around SVG icon
+    'margin': 0.5em,
+    'border-radius': 2px
+  );
+  $-options: map.merge($defaultOptions, $options);
 
-// Set hover colors and transition for a button
-@mixin hover-state($colormap) {
-  &:hover:not([disabled]),
-  &:focus:not([disabled]) {
-    color: map.get($colormap, 'hover-foreground');
-    background-color: map.get($colormap, 'hover-background');
-    @content;
-  }
-  transition: color 0.2s ease-out, background-color 0.2s ease-out,
-    opacity 0.2s ease-out;
-}
+  @include base.button($options: $-options);
 
-// Set active state colors for a button
-@mixin active-state($colormap) {
-  &[aria-expanded='true'],
-  &[aria-pressed='true'] {
-    color: map.get($colormap, 'active-foreground');
-    background-color: map.get($colormap, 'active-background');
-    @content;
-
-    &:hover:not([disabled]),
-    &:focus:not([disabled]) {
-      color: map.get($colormap, 'active-foreground');
-      background-color: map.get($colormap, 'active-background');
-      @content;
+  // Add styles for supported variants as modifier classes, if `withVariants` enabled
+  @if (map.get($-options, 'withVariants')) {
+    &--light {
+      $-light-options: (
+        'colormap': map.get($-options, 'colormap--light'),
+        'withStates': map.get($-options, 'withStates'),
+      );
+      @include base.button--variant($-light-options);
+    }
+
+    &--primary {
+      $-primary-options: (
+        'colormap': map.get($-options, 'colormap--primary'),
+        'withStates': map.get($-options, 'withStates'),
+      );
+      @include base.button--variant($-primary-options);
     }
-  }
-}
 
-// Variant mixin: may be be used by variants (BEM modifier classes)
-@mixin _variant($options) {
-  @include _colors(map.get($options, 'colormap'));
-  @if map.get($options, 'withStates') {
-    @include active-state(map.get($options, 'colormap'));
-    @include hover-state(map.get($options, 'colormap'));
+    &--dark {
+      $-dark-options: (
+        'colormap': map.get($-options, 'colormap--dark'),
+        'withStates': map.get($-options, 'withStates'),
+      );
+      @include base.button--variant($-dark-options);
+    }
   }
   @content;
 }
 
-// Base mixin for buttons.
-@mixin _button($options) {
-  @include reset;
-  @include _colors(map.get($options, 'colormap'));
-
-  @if map.get($options, 'withStates') {
-    @include active-state(map.get($options, 'colormap'));
-    @include hover-state(map.get($options, 'colormap'));
-  }
-
-  border-radius: map.get($options, 'border-radius');
-  border: none;
-  padding: 0.5em;
-
-  font-size: 1em;
-  font-weight: 700;
-  white-space: nowrap; // Keep multi-word button labels from wrapping
-
-  display: flex;
-  justify-content: center;
-  align-items: center;
-
-  @if map.get($options, 'withLayout') {
-    &--icon-left svg {
-      margin-right: map.get($options, 'margin');
-    }
+// Base mixin for a button with an icon and no label/content. Supports
+// variants and sizes. Will assert responsive touch-target sizing in
+// --medium (default) and --large variants.
+@mixin IconButton($options: ()) {
+  $defaultOptions: (
+    'colormap': c.$IconButton-colors,
+    'colormap--light': c.$IconButton-colors--light,
+    'colormap--primary': c.$IconButton-colors--primary,
+    'colormap--dark': c.$IconButton-colors--dark,
+    'responsive': true,
+  );
+  $-options: map.merge($defaultOptions, $options);
 
-    &--icon-right svg {
-      margin-left: map.get($options, 'margin');
-    }
-    // When a button has "layout", that indicates it has some textual content:
-    // Size text to the contextual 1em, and adjust the icon to look balanced.
-    // H frontend app buttons tend to apply an icon:text ratio of ~1.25:1
-    svg {
-      width: 1.25em;
-      height: 1.25em;
-    }
-  } @else {
-    // In the case where an icon is the only content in a <button> element,
-    // size the icon based on contextual font-size. i.e. the icon IS
-    // the content
-    svg {
-      width: 1em;
-      height: 1em;
+  @include Button($-options) {
+    // Establish a minimum touch-target for touch devices if 'responsive'
+    // option is enabled (default). This is not applied to the `--small`
+    // size variant.
+    @if map.get($-options, 'responsive') {
+      @media (pointer: coarse) {
+        &--medium,
+        &--large {
+          min-width: c.$touch-target-size;
+          min-height: c.$touch-target-size;
+        }
+      }
     }
   }
-}
 
-/**
- * Convenience function to retrieve a colormap. This obviates the need to reach
- * into the `_config` module directly from outside and is less verbose.
- *
- * @param {'colors'|'icon'|'icon--primary'|'labeled'|'labeled--primary'|'link'}
- *        [$name] - Shorthand name of colormap to retrieved. Defaults to base
- *          $colors.
- * @return {sass:map}
- */
-@function colormap($name: 'colors') {
-  $result: $colors;
-  @if $name == 'icon' {
-    $result: $IconButton-colors;
-  }
-  @if $name == 'icon--primary' {
-    $result: $IconButton-colors--primary;
-  }
-  @if $name == 'labeled' {
-    $result: $LabeledButton-colors;
-  }
-  @if $name == 'labeled--primary' {
-    $result: $LabeledButton-colors--primary;
-  }
-  @if $name == 'link' {
-    $result: $LinkButton-colors;
-  }
-  @return $result;
+  @content;
 }
 
-// Base mixin for <button> elements. Start here for new <button> classes
-@mixin Button($options: ()) {
+// Base mixin for a button that has text/content and, optionally, an icon.
+// Supports variants and sizes.
+@mixin LabeledButton($options: ()) {
   $defaultOptions: (
-    // What colors should be used for this button's styling?
-    'colormap': $colors,
-    // Should styling be added for "active" and "hover" states for this <button>?
-    'withStates': true,
-    // Should styling be added to support the layout of multiple sub-elements
-    // of this <button>? Not needed if <button> contains only one child.
-    'withLayout': false,
-    // Internal margin around SVG icon
-    'margin': 0.5em,
-    'border-radius': 2px
+    'colormap': c.$LabeledButton-colors,
+    'colormap--light': c.$LabeledButton-colors--light,
+    'colormap--primary': c.$LabeledButton-colors--primary,
+    'colormap--dark': c.$LabeledButton-colors--dark,
+    'withLayout': true,
   );
   $-options: map.merge($defaultOptions, $options);
-  @include _button($options: $-options);
+
+  @include Button($-options);
+
   @content;
 }
 
-// Mixin for --modifier variants on buttons, e.g. `.MyButton--primary`
-@mixin Button--variant($options: ()) {
+// Base mixin for a button styled to look like an <a> link. Supports variants
+// but not pressed/active states at present.
+@mixin LinkButton($options: ()) {
   $defaultOptions: (
-    // Colors to use when styling this variant
-    'colormap': $colors,
-    // Should this variant have styling for "active" and "hover" states?
-    'withStates': true
+    'colormap': c.$LinkButton-colors,
+    'colormap--light': c.$LinkButton-colors--light,
+    'colormap--primary': c.$LinkButton-colors--primary,
+    'colormap--dark': c.$LinkButton-colors--dark,
   );
   $-options: map.merge($defaultOptions, $options);
-  @include _variant($options: $-options);
+  @include Button($-options) {
+    // Lighter font weight for link-styled buttons
+    font-weight: 400;
+    // Add an underline on hover
+    &:hover:not([disabled]) {
+      text-decoration: underline;
+    }
+
+    &--primary {
+      // Primary variant has bolder text (and also different colors)
+      font-weight: 500;
+    }
+  }
+
+  // Remove padding to allow button to appear flush with surrounding content
+  padding: 0;
   @content;
 }

src/styles/sidebar/buttons.scss

 // Button styling for the sidebar extending common button-component styles
 @use '../shared/components/buttons/mixins' as buttons;
 
-// Similar to `.IconButton`, with these changes:
-// - omit responsive minimum sizing
-// - tighten padding
-.CompactIconButton {
-  // Use icon colors for base and primary variants
-  $base-options: (
-    'colormap': buttons.colormap('icon'),
+// Similar to `.LinkButton`, with inline layout (so button can be used
+// within text)
+.InlineLinkButton {
+  @include buttons.LinkButton(
+    (
+      'inline': true,
+    )
   );
-  $primary-options: (
-    'colormap': buttons.colormap('icon--primary'),
-  );
-
-  @include buttons.Button($options: $base-options) {
-    padding: 0.25em; // Override padding
-  }
-  &--primary {
-    @include buttons.Button--variant($options: $primary-options);
+  // Custom: The dark variant is used for inline, anchor-like styling and needs
+  // to be underlined at all times to match <a> styling near it
+  &--dark {
+    text-decoration: underline;
   }
 }

src/styles/util.scss

@@ -50,6 +50,10 @@
   @include layout.vertical-rhythm;
 }
 
+.u-font--large {
+  @include utils.font--large;
+}
+
 .u-font--xlarge {
   @include utils.font--xlarge;
 }