Commit 176d3096 authored by Lyza Danger Gardner's avatar Lyza Danger Gardner

Refactor `Button` component styling

Handling any deviation from default styling of `Button` components
has been painful, and these changes make it an all or nothing affair:
the `Button` component will apply default styling only in the absence
of a `className` prop. When a `className` prop is present, that will
supersede any component default styling.

These changes make button SCSS mixins clearer and easier to reuse and
extend.

`usePrimaryStyle`, `useCompactStyle`, `useInputStyle` props removed
from `Button` in preference of mixins to reduce `Button` complexity and
confusion over CSS precedence.

Components that use `Button`, and their styles, have been updated here
as needed.
parent f3fd4aba
......@@ -102,7 +102,7 @@ function AnnotationPublishControl({
icon="cancel"
buttonText="Cancel"
onClick={onCancel}
useCompactStyle
className="annotation-publish-control__btn-cancel"
/>
</div>
);
......
......@@ -87,7 +87,12 @@ function AnnotationShareControl({
return (
<div className="annotation-share-control" ref={shareRef}>
<Button icon="share" title="Share" onClick={toggleSharePanel} />
<Button
icon="share"
title="Share"
onClick={toggleSharePanel}
isExpanded={isOpen}
/>
{isOpen && (
<div className="annotation-share-panel">
<div className="annotation-share-panel__header">
......@@ -109,8 +114,7 @@ function AnnotationShareControl({
icon="copy"
title="Copy share link to clipboard"
onClick={copyShareLink}
useInputStyle
useCompactStyle
className="annotation-share-panel__icon-button"
/>
</div>
<div className="annotation-share-panel__permissions">
......
import classnames from 'classnames';
import { createElement } from 'preact';
import propTypes from 'prop-types';
import SvgIcon from '../../shared/components/svg-icon';
/**
* A button, one of three base types depending on provided props:
* - Icon-only button: `icon` present, `buttonText` missing
* - Text-only button: `buttonText` present, `icon` missing
* - Icon and text: both `icon` and `buttonText` present
* A button.
*
* Buttons may be additionally styled with 0 to n of (none are mutually exclusive):
* - `useCompactStyle`: for fitting into tighter spaces
* - `useInputStyle`: for placing an icon-only button next to an input field
* - `usePrimaryStyle`: for applying "primary action" styling
* - `className`: arbitrary additional class name(s) to apply
* By default, styling will be applied for the button based on its general
* "type" (e.g. an icon-only button, a labeled button). The presence of a
* `className` prop will disable all default styling.
*/
export default function Button({
buttonText = '',
......@@ -26,15 +20,14 @@ export default function Button({
onClick = () => null,
style = {},
title,
useCompactStyle = false,
useInputStyle = false,
usePrimaryStyle = false,
}) {
// If `buttonText` is provided, the `title` prop is optional and the `button`'s
// `title` attribute will be set from the `buttonText`
title = title || buttonText;
const baseClassName = buttonText ? 'button--labeled' : 'button--icon-only';
const buttonModifier = buttonText ? 'labeled' : 'icon-only';
// `className` overrides default class naming when present
const baseClassName = className || `button--${buttonModifier}`;
const extraProps = {};
......@@ -54,23 +47,13 @@ export default function Button({
return (
<button
className={classnames(
'button',
baseClassName,
{
'button--compact': useCompactStyle,
'button--input': useInputStyle,
'button--primary': usePrimaryStyle,
'is-active': isPressed,
},
className
)}
className={baseClassName}
onClick={onClick}
style={style}
disabled={disabled}
{...extraProps}
>
{icon && <SvgIcon name={icon} className="button__icon" />}
{icon && <SvgIcon name={icon} />}
{buttonText}
</button>
);
......@@ -108,9 +91,8 @@ Button.propTypes = {
buttonText: propTypes.string,
/**
* optional CSS classes to add to the `button` element. These classes may
* control color, etc., but should not define padding or layout, which are
* owned by this component
* When present, this will be used as the base class name and will override
* all styling. See `buttons` SCSS mixins module for more details.
*/
className: propTypes.string,
......@@ -146,16 +128,4 @@ Button.propTypes = {
* `title`, used for button `title`, is required unless `buttonText` is present
*/
title: requiredStringIfButtonTextMissing,
/** Allows a variant of button that takes up less space */
useCompactStyle: propTypes.bool,
/** Allows a variant of button that can sit right next to an input field */
useInputStyle: propTypes.bool,
/**
* Does this button represent the "primary" action available? If so,
* differentiating styles will be applied.
*/
usePrimaryStyle: propTypes.bool,
};
......@@ -30,9 +30,8 @@ export default function LoginPromptPanel({ onLogin, onSignUp }) {
/>
<Button
buttonText="Log in"
className="sidebar-panel__button"
className="sidebar-panel__button--primary"
onClick={onLogin}
usePrimaryStyle
/>
</div>
</SidebarPanel>
......
......@@ -30,11 +30,10 @@ function NewNoteButton({ annotationsService, settings }) {
<div className="new-note-button">
<Button
buttonText="New note"
className="button--primary"
icon="add"
onClick={onNewNoteBtnClick}
style={applyTheme(['ctaBackgroundColor'], settings)}
useCompactStyle
usePrimaryStyle
/>
</div>
);
......
......@@ -66,7 +66,6 @@ export default function SearchInput({ alwaysExpanded, query, onSearch }) {
icon="search"
onClick={() => input.current.focus()}
title="Search annotations"
useCompactStyle
/>
)}
{isLoading && <Spinner className="top-bar__btn" title="Loading…" />}
......
......@@ -145,8 +145,7 @@ function SearchStatusBar({ rootThread }) {
icon="cancel"
buttonText="Clear search"
onClick={actions.clearSelection}
useCompactStyle
usePrimaryStyle
className="search-status-bar__button"
/>
<span className="search-status-bar__filtered-text">
{modeText.filtered}
......@@ -165,8 +164,7 @@ function SearchStatusBar({ rootThread }) {
<Button
buttonText={modeText.selected}
onClick={actions.clearSelection}
useCompactStyle
usePrimaryStyle
className="search-status-bar__button"
/>
</div>
)}
......
......@@ -80,7 +80,7 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
icon="copy"
onClick={copyShareLink}
title="Copy share link"
useInputStyle
className="share-annotations-panel__icon-button"
/>
</div>
<p>
......
import { createElement } from 'preact';
import classnames from 'classnames';
import propTypes from 'prop-types';
import useStore from '../store/use-store';
......@@ -50,17 +51,18 @@ export default function SidebarContentError({
{showClearSelection && (
<Button
buttonText="Show all annotations"
className="sidebar-content-error__button"
className={classnames({
'sidebar-content-error__button': !isLoggedIn,
'sidebar-content-error__button--primary': isLoggedIn,
})}
onClick={clearSelection}
usePrimaryStyle={isLoggedIn}
/>
)}
{!isLoggedIn && (
<Button
buttonText="Log in"
className="sidebar-content-error__button"
className="sidebar-content-error__button--primary"
onClick={onLoginRequest}
usePrimaryStyle
/>
)}
</div>
......
......@@ -57,12 +57,7 @@ export default function SidebarPanel({
)}
<h2 className="sidebar-panel__title u-stretch">{title}</h2>
<div>
<Button
icon="cancel"
buttonText="Close"
onClick={closePanel}
useCompactStyle
/>
<Button icon="cancel" buttonText="Close" onClick={closePanel} />
</div>
</div>
<div className="sidebar-panel__content">{children}</div>
......
......@@ -30,12 +30,6 @@ describe('Button', () => {
$imports.$restore();
});
it('adds active className if `isPressed` is `true`', () => {
const wrapper = createComponent({ isPressed: true });
assert.isTrue(wrapper.find('button').hasClass('is-active'));
});
it('renders `SvgIcon` for associated icon', () => {
const wrapper = createComponent();
assert.equal(wrapper.find('SvgIcon').prop('name'), 'fakeIcon');
......@@ -115,28 +109,26 @@ describe('Button', () => {
assert.calledOnce(fakeOnClick);
});
it('adds additional class name passed in `className` prop', () => {
const wrapper = createComponent({ className: 'my-class' });
assert.isTrue(wrapper.hasClass('my-class'));
});
it('sets compact style if `useCompactStyle` is set`', () => {
const wrapper = createComponent({ useCompactStyle: true });
it('applies icon-only class if no visible label present', () => {
const wrapper = createComponent();
assert.isTrue(wrapper.find('button').hasClass('button--compact'));
assert.isTrue(wrapper.find('button').hasClass('button--icon-only'));
assert.isFalse(wrapper.find('button').hasClass('button--labeled'));
});
it('sets input style if `useInputStyle` is set', () => {
const wrapper = createComponent({ useInputStyle: true });
it('applies labeled class if any visible label present', () => {
const wrapper = createComponent({ buttonText: 'Some text' });
assert.isTrue(wrapper.find('button').hasClass('button--input'));
assert.isTrue(wrapper.find('button').hasClass('button--labeled'));
assert.isFalse(wrapper.find('button').hasClass('button--icon-only'));
});
it('sets primary style if `usePrimaryStyle` is set`', () => {
const wrapper = createComponent({ usePrimaryStyle: true });
it('applies class passed in `className` prop', () => {
const wrapper = createComponent({ className: 'my-class' });
assert.isTrue(wrapper.find('button').hasClass('button--primary'));
assert.isTrue(wrapper.find('button').hasClass('my-class'));
assert.isFalse(wrapper.hasClass('button--icon-only'));
assert.isFalse(wrapper.hasClass('button--labeled'));
});
it('disables the button when `disabled` prop is true', () => {
......
......@@ -36,9 +36,7 @@ describe('SidebarContentError', () => {
});
const findButtonByText = (wrapper, text) => {
return wrapper
.find('.sidebar-content-error__button')
.filter({ buttonText: text });
return wrapper.find('Button').filter({ buttonText: text });
};
const errorText = wrapper => {
......
......@@ -108,7 +108,6 @@ function TopBar({
isExpanded={currentActivePanel === uiConstants.PANEL_HELP}
onClick={requestHelp}
title="Help"
useCompactStyle
/>
{loginControl}
</div>
......@@ -126,7 +125,6 @@ function TopBar({
title={`Show ${pendingUpdateCount} new/updated ${
pendingUpdateCount === 1 ? 'annotation' : 'annotations'
}`}
useCompactStyle
/>
)}
<SearchInput query={filterQuery} onSearch={setFilterQuery} />
......@@ -140,7 +138,6 @@ function TopBar({
}
onClick={toggleSharePanel}
title="Share annotations on this page"
useCompactStyle
/>
)}
<Button
......@@ -149,7 +146,6 @@ function TopBar({
isExpanded={currentActivePanel === uiConstants.PANEL_HELP}
onClick={requestHelp}
title="Help"
useCompactStyle
/>
{loginControl}
</div>
......
/**
* Button mixins for use by the `Button` component or any component that wishes
* to override default `Button`-component styling. These mixins are meant to be
* applied to `<button>` HTML elements, with an optional contained icon
* (i.e. `SvgIcon` component or `<svg>` element).
*
* To customize default `Button` styling, start with an appropriate button mixin
* and extend or or override rules as necessary in your component's SCSS module.
*
* e.g., let's pretend you wish the `Button` used in `MyComponent`
* to have a pink background. In `my-component.scss`, you might do something like:
*
* .my-component-button {
* @include button--labeled;
* background-color: pink;
* }
*
* And pass 'my-component-button' as the `className` prop to `Button`.
*/
@use "./focus";
@use "../variables" as var;
@mixin reset-native-btn-styles {
@include focus.outline-on-keyboard-focus;
padding: 0px;
margin: 0px;
padding: 0;
margin: 0;
background-color: transparent;
border-style: none;
}
/**
* Basic color, sizing, padding and hover for any button
* Basic color, sizing, padding and hover for buttons.
*/
@mixin button-base {
@mixin button {
@include reset-native-btn-styles;
padding: 0.5em;
color: var.$grey-mid;
&__icon {
display: flex;
align-items: center;
justify-content: center;
border-radius: 2px;
border: none;
// Icon
svg {
width: 16px;
height: 16px;
}
......@@ -27,8 +52,18 @@
transition: 0.2s ease-out;
color: var.$grey-7;
}
}
&.is-active {
/* A button with an icon and no displayed text */
@mixin button--icon-only {
@include button;
color: var.$grey-mid;
@media (pointer: coarse) {
min-width: var.$touch-target-size;
min-height: var.$touch-target-size;
}
&[aria-expanded='true'],
&[aria-pressed='true'] {
color: var.$color-brand;
&:hover {
......@@ -37,28 +72,29 @@
}
}
/**
* A button with background color, layout rules to allow an icon, and text settings
*/
@mixin button-basic {
display: flex;
align-items: center;
border-radius: 2px;
border: none;
background-color: var.$grey-1;
font-weight: 700;
/* A button with displayed text. It may or may not have an icon. */
@mixin button--labeled {
@include button;
color: var.$grey-mid;
padding-right: 0.75em;
padding-left: 0.75em;
font-weight: 700;
background-color: var.$grey-1;
&:hover {
background-color: var.$grey-2;
}
// Icon
svg {
margin: 0 5px 0 0;
}
}
/* colors for a "primary" button */
@mixin button-primary {
/**
* A labeled button that is a primary action.
*/
@mixin button--primary {
@include button;
@include button--labeled;
color: var.$grey-1;
background-color: var.$grey-mid;
......@@ -67,3 +103,35 @@
background-color: var.$grey-6;
}
}
/**
* An icon-only button that sits to the right of a text-input field
* (e.g. "copy to clipboard" button in share panels)
*/
@mixin button--input {
@include button;
color: var.$grey-mid;
padding: 0.5em 0.75em;
background-color: var.$grey-1;
border-radius: 0; // Turn off border-radius to align with <input> edges
border: 1px solid var.$grey-3;
border-left: 0; // Avoid double border with the <input>
&:hover {
background-color: var.$grey-2;
}
}
/**
* A button that is styled roughly like an `<a>` element (link-like)
*/
@mixin button--link {
@include button;
color: var.$grey-mid;
padding: 0;
&:hover {
color: var.$color-link-hover;
text-decoration: underline;
}
}
@use "buttons";
@use "../variables" as var;
@use "./buttons";
/**
* Base styles for a "panel"-like element, with appropriate
......@@ -46,6 +46,12 @@
}
&__button {
@include buttons.button--labeled;
margin-left: 1em;
}
&__button--primary {
@include buttons.button--primary;
margin-left: 1em;
}
......
@use '../../variables' as var;
@use "../../mixins/buttons";
.annotation-body {
@include var.font-normal;
......@@ -38,6 +39,7 @@
justify-content: flex-end;
.annotation-body__collapse-toggle-button {
@include buttons.button--labeled;
padding: 0.5em;
background-color: transparent;
}
......
@use "../../mixins/buttons";
@use "../../mixins/forms";
@use "../../variables" as var;
......@@ -17,14 +18,9 @@
color: var.$color-text;
}
&__reply-toggle.button {
&__reply-toggle {
@include buttons.button--link;
padding: 0 0.5em;
font-weight: 400;
background-color: transparent;
&:hover {
background-color: transparent;
color: var.$color-link-hover;
}
}
// Timestamps are right aligned in a flex row
......
@use "../../mixins/buttons";
@use "../../mixins/forms";
@use "../../variables" as var;
......@@ -31,6 +32,16 @@
min-width: 100%;
}
&-cancel {
@include buttons.button--labeled;
padding-right: 0.5em;
padding-left: 0.5em;
&__icon {
margin: 0;
}
}
&-primary {
@include forms.primary-action-btn;
......
@use '../../mixins/buttons';
@use '../../mixins/links';
@use '../../mixins/panel';
@use "../../variables" as var;
......@@ -19,6 +20,15 @@
display: flex;
}
&__icon-button {
@include buttons.button--input;
padding: 0.25em 0.5em;
&__icon {
margin: 0;
}
}
& .form-input {
padding: 0.5em;
border-radius: 0;
......
@use "../../mixins/buttons";
@use "../../variables" as var;
// FIXME These hover-related rules should live elsewhere
......@@ -37,15 +38,8 @@
}
}
&__reply-toggle.button {
background-color: transparent;
padding: 0;
font-weight: 400;
&:hover {
background-color: transparent;
text-decoration: underline;
}
&__reply-toggle {
@include buttons.button--link;
}
&__controls {
......
......@@ -2,73 +2,23 @@
@use "../../variables" as var;
/**
* Expected button markup structure:
* Button markup structure as generated by the `<Button>` component when
* default styles are not overridden:
*
* <button class="button <button--icon-only|button--labeled> [button--compact] [button--primary] [is-active]">
* [<SvgIcon class="button__icon" />]
* <button class="<button--icon-only|button--labeled>">
* [<SvgIcon />]
* [<span>label</span>]
* </button>
*/
.button {
@include buttons.button-base;
}
.button--labeled {
@include buttons.button-basic;
// Need a little space around the icon if a label is next to it
.button__icon {
margin: 0 5px;
}
}
/* A button that sits right next to a (text) input and feels "at one" with it */
.button--input {
@include buttons.button-basic;
padding: 10px;
border-radius: 0; // Turn off border-radius to align with <input> edges
border: 1px solid var.$grey-3;
border-left: 0px; // Avoid double border with the <input>
}
// In a few cases, it's necessary to squeeze buttons into tight spots
.button--compact {
// e.g. in the top bar, need to have icons right next to each other
&.button--icon-only {
padding: 0.25em;
}
// e.g. when next to an input field in a small/tight component
&.button--input {
padding: 0.25em 0.5em;
}
// e.g. the "close" button for sidebar panels, other tighter spaces
&.button--labeled {
padding-right: 0.5em;
padding-left: 0.5em;
.button__icon {
margin: 0;
}
}
.button {
@include buttons.button;
}
.button--primary {
@include buttons.button-primary;
.button--icon-only {
@include buttons.button--icon-only;
}
@media (pointer: coarse) {
.button {
min-width: var.$touch-target-size;
min-height: var.$touch-target-size;
}
// Until the top bar can be refactored to allow for breathing room around
// the search interface, we can't spare the room for comfortable tap targets
// on touchscreen devices. This overrides `Button`'s larger tap targets.
.button--compact {
min-width: auto;
min-height: auto;
}
.button--labeled {
@include buttons.button--labeled;
}
@use "../../mixins/buttons";
@use "../../variables" as var;
.logged-out-message {
......@@ -10,17 +11,11 @@
}
.logged-out-message__link {
@include buttons.button--link;
padding: 0;
display: inline;
color: var.$color-text;
background-color: transparent;
text-decoration: underline;
font-weight: 400;
&:hover {
color: var.$color-text;
text-decoration: none;
}
}
.logged-out-message__logo {
......
......@@ -23,6 +23,10 @@
transition: 0.2s ease-out;
color: var.$color-text;
}
&[aria-expanded='true'],
&:hover[aria-expanded='true'] {
color: var.$color-brand;
}
}
.menu__toggle-wrapper {
......@@ -45,6 +49,7 @@
&.is-open {
// Flip the indicator when the menu is open.
transform: rotateX(180deg);
color: var.$color-text;
}
}
......
@use '../../mixins/buttons';
.search-status-bar {
display: flex;
align-items: center;
margin-bottom: 10px;
}
.search-status-bar > button {
.search-status-bar__button {
@include buttons.button--primary;
padding-right: 0.75em;
margin-right: 10px;
}
@use "../../variables" as var;
@use "../../mixins/buttons";
@use "../../mixins/links";
.share-annotations-panel {
......@@ -23,6 +23,10 @@
height: 1em;
}
&__icon-button {
@include buttons.button--input;
}
.share-annotations-panel-links {
@include links.footer-links;
......
......@@ -32,7 +32,8 @@
}
&__delete {
@include buttons.button-base;
@include buttons.button;
color: var.$grey-mid;
background-color: var.$grey-1;
padding: 0 0.5em;
border: 1px solid var.$grey-3;
......
@use "../../mixins/buttons";
@use "../../variables" as var;
.thread {
......@@ -30,29 +31,29 @@
}
}
// TODO These styles should be consolidated with other `Button` styles
&__collapse-button {
@include buttons.button;
color: var.$grey-mid;
margin-right: -1.25em;
padding: 0.25em 0.75em 0.75em 0.75em;
// Need a non-transparent background so that the dashed border line
// does not show through the button
background-color: var.$white;
.button__icon {
svg {
width: 12px;
height: 12px;
color: var.$grey-4;
}
&:hover {
.button__icon {
color: var.$grey-6;
}
&:hover &__icon {
color: var.$grey-6;
}
}
&__hidden-toggle-button {
// This makes the vertical alignment with thread-collapse chevrons
// more precise
@include buttons.button--labeled;
margin-top: -0.25em;
}
......
......@@ -3,6 +3,7 @@
@use "../../variables" as var;
.top-bar {
color: var.$grey-mid;
background: var.$white;
border-bottom: solid 1px var.$grey-3;
height: var.$top-bar-height;
......@@ -18,8 +19,19 @@
// the window is scrolled.
transform: translate3d(0, 0, 0);
&__icon-button,
&__menu-icon {
&__icon-button {
@include buttons.button--icon-only;
padding: 0.25em;
@media (pointer: coarse) {
// Until the top bar can be refactored to allow for breathing room around
// the search interface, we can't spare the room for comfortable tap targets
// on touchscreen devices. This overrides `Button`'s larger tap targets.
min-width: auto;
min-height: auto;
}
}
&__icon-button {
color: var.$grey-mid;
&.is-active {
......@@ -48,16 +60,20 @@
border-bottom: none;
}
.top-bar__login-button.button {
.top-bar__login-button {
@include buttons.button;
padding: 0 0.25em;
background-color: transparent;
color: var.$color-brand;
font-size: var.$body2-font-size;
font-weight: 400;
&:hover {
color: var.$color-link-hover;
}
@media (pointer: coarse) {
min-width: inherit;
min-height: inherit;
}
}
.top-bar__login-links {
......
@use "../../variables" as var;
@use "../../mixins/buttons";
.version-info {
margin-top: 0.5em;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment