From 9ebd6cd01f3ddb190c4cc20d49fc7ed96fe3436b Mon Sep 17 00:00:00 2001 From: langermank Date: Thu, 13 Jan 2022 09:13:51 -0800 Subject: [PATCH 01/16] begin button api refactor --- src/button-refactor/ButtonsLinksAPI.md | 68 ++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 src/button-refactor/ButtonsLinksAPI.md diff --git a/src/button-refactor/ButtonsLinksAPI.md b/src/button-refactor/ButtonsLinksAPI.md new file mode 100644 index 0000000000..7150d13046 --- /dev/null +++ b/src/button-refactor/ButtonsLinksAPI.md @@ -0,0 +1,68 @@ +# Button and Link component APIs +Button and Link has been discussed in a number of issues and discussions over past few years. I went through the list below, took note of their purpose and started compiling a new API spec. This doc is is meant to be collaborative and accompany Storybook docs while we work through a final API recommendation. Once we have a solid plan, this can move into an issue and be delegated across frameworks. + +Existing issues +[x] https://github.com/github/primer/issues/141 +- Accessory button component, should reconcile with reaction button (sister component to Button) +[x] https://github.com/github/primer/issues/220 +- Tracking issue, mentions Button in the context of API consistency +[x] https://github.com/github/primer/issues/253 +- Button audit, anatomy spec, design considerations, some button link discussion, icon buttons (this issue is very informative!) +[x] https://github.com/github/primer/issues/263 +- More design discussion, particularly focused on outline button +[x] https://github.com/github/primer/issues/272 +- Make icon only buttons square +[x] https://github.com/github/primer/issues/321 +- Super specific button use case? +[x] https://github.com/github/primer/issues/350 +- a11y audit with task list for PVC +[x] https://github.com/github/primer/issues/382 +- React button refactor +[x] https://github.com/github/primer/issues/468 +- Visual bugs (colors, state) +[x] https://github.com/github/primer/discussions/87 +- Discussion about accessory buttons +[x] https://github.com/github/primer/discussions/211 +- Open ended discussion about primary button as dropdown +[x] https://github.com/github/primer/discussions/459 +- Allie's thoughts on limiting icon only buttons and working towards encouraging visual labels +[x] https://github.com/github/primer/discussions/477 +- Buttons styled as links, links styled as buttons + +## Specific notes from previous bugs to keep in mind +[] Check all button variants have shadow present +[] Ensure icon colors are consistent in hover states +[] Ensure icon colors are consistent with variants in all states +[] Ensure disabled colors are consistent across frameworks + +## Component list +A few discussions were about naming and prop drilling (source here). We found that for Button to handle all of its use cases, we would need a large number of props– some conditional and dependent on one another. When that happens, it's typically a sign that the logic should be separated into another component. + +| Component | Description | +| -- | -- | +| Button | standard `button` with variants, size, visual slots | +| IconButton | `button` with icon only (square) and required `aria-label` | +| ButtonStyledAsLink | `button` that visually looks like a link | +| ReactionButton | `button` snowflake with specific styles/interaction design | +| ButtonGroup | wrapper to handle grouping buttons | +| Link | `a` with variants, optional trailing visuals | +| LinkStyledAsButton | `a` with button variants, required trailing visuals | + +## Component API breakdown + +### Button + +| prop | type | options | default | notes | +| -- | -- | -- | -- | -- | +| `variant` | one-of string | `primary` `secondary` `danger` `outline`? | `secondary` | | +| `size` | one-of string | `small` `default` `large` | `default | | +| `label` | string | button description | null | | +| `aria-label` | string | button description for screen readers | null | | +| `aria-pressed` | boolean | `true/false` | `false` | | +| `leadingVisual` | children (slot) | octicon | null | | +| `trailingVisual` | children (slot) | octicon | null | | +| `trailingAction` | children (slot) | octicon | null | slot for caret to maintain leading/trailing visuals if they exist | +| `fullWidth` | boolean | `true/false` | `false` | | +| `visualPosition` | one-of string | `fixed` `some-other-word` | `fixed` | lock icon to the text label or to the button container | + +## Component design specs From f15a0ac382cbf9bfdda0ea36ec20d2d13aa652c1 Mon Sep 17 00:00:00 2001 From: langermank Date: Thu, 13 Jan 2022 17:33:09 -0800 Subject: [PATCH 02/16] setup button playground --- .../stories/explorations/Button.stories.jsx | 172 +++++++++++++++++ src/button-refactor/button.scss | 181 ++++++++++++++++++ src/button-refactor/link.scss | 0 src/core/index.scss | 2 +- 4 files changed, 354 insertions(+), 1 deletion(-) create mode 100644 docs/src/stories/explorations/Button.stories.jsx create mode 100644 src/button-refactor/button.scss create mode 100644 src/button-refactor/link.scss diff --git a/docs/src/stories/explorations/Button.stories.jsx b/docs/src/stories/explorations/Button.stories.jsx new file mode 100644 index 0000000000..d9ecc7e234 --- /dev/null +++ b/docs/src/stories/explorations/Button.stories.jsx @@ -0,0 +1,172 @@ +import React from 'react' +import clsx from 'clsx' + +export default { + title: 'Explorations/Button', + parameters: { + // design: { + // type: 'figma', + // url: 'https://www.figma.com/file/GCvY3Qv8czRgZgvl1dG6lp/Primer-Web?node-id=4371%3A7128' + // }, + layout: 'padded' + }, + + excludeStories: ['ButtonTemplate'], + argTypes: { + variant: { + options: [0, 1, 2, 3], // iterator + mapping: [null, 'Button-primary', 'Button-outline', 'Button-danger'], // values + control: { + type: 'select', + labels: ['default', 'primary', 'outline', 'danger'] + }, + table: { + category: 'CSS' + } + }, + size: { + options: [0, 1, 2], // iterator + mapping: [null, 'Button-small', 'Button-large'], // values + control: { + type: 'select', + labels: ['default', 'small', 'large'] + }, + table: { + category: 'CSS' + } + }, + label: { + defaultValue: 'Button', + type: 'string', + name: 'label', + description: 'string', + table: { + category: 'HTML' + } + }, + disabled: { + defaultValue: false, + control: {type: 'boolean'}, + table: { + category: 'Interactive' + } + }, + fullWidth: { + defaultValue: false, + control: {type: 'boolean'}, + table: { + category: 'CSS' + } + }, + leadingVisual: { + name: 'leadingVisual', + control: {type: 'boolean'}, + description: 'Paste [Octicon](https://primer.style/octicons/) in control field', + table: { + category: 'HTML' + } + }, + trailingVisual: { + name: 'trailingVisual', + control: {type: 'boolean'}, + description: 'Paste [Octicon](https://primer.style/octicons/) in control field', + table: { + category: 'HTML' + } + }, + trailingAction: { + defaultValue: false, + control: {type: 'boolean'}, + table: { + category: 'CSS' + } + }, + pressed: { + defaultValue: false, + control: {type: 'boolean'}, + table: { + category: 'CSS' + } + }, + focusElement: { + control: {type: 'boolean'}, + description: 'set focus on one element', + table: { + category: 'Interactive' + } + }, + focusAllElements: { + control: {type: 'boolean'}, + description: 'set focus on all elements for viewing in all themes', + table: { + category: 'Interactive' + } + } + } +} + +const focusMethod = function getFocus() { + // find the focusable element + var button = document.getElementsByTagName('button')[0] + // set focus on element + button.focus() +} + +const mona = ( + + + +) + +const caret = ( + + + +) + +export const ButtonTemplate = ({ + label, + variant, + disabled, + size, + fullWidth, + leadingVisual, + trailingVisual, + trailingAction, + pressed, + focusElement, + focusAllElements +}) => ( + <> + + {focusElement && focusMethod()} + +) + +export const Playground = ButtonTemplate.bind({}) +Playground.args = { + focusElement: false, + focusAllElements: false, + leadingVisual: false, + trailingAction: false, + trailingVisual: false +} diff --git a/src/button-refactor/button.scss b/src/button-refactor/button.scss new file mode 100644 index 0000000000..4251fdbd97 --- /dev/null +++ b/src/button-refactor/button.scss @@ -0,0 +1,181 @@ +.Button { + position: relative; + font-size: $body-font-size; + font-weight: $font-weight-semibold; + cursor: pointer; + user-select: none; + border: $border-width $border-style; + border-radius: $border-radius; + color: var(--color-btn-text); + background-color: var(--color-btn-bg); + border-color: var(--color-btn-border); + box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); + transition: 0.2s cubic-bezier(0.3, 0, 0.5, 1); + transition-property: color, background-color, border-color; + height: 32px; + padding: 0 16px; + display: grid; + grid-template-areas: 'leadingIcon text trailingIcon trailingAction'; + grid-template-columns: min-content minmax(0, auto) min-content min-content; + align-items: center; + + // column-gap persists with empty grid-areas, margin applies only when children exist + > :not(:last-child) { + margin-right: $spacer-2; + } + + &:hover, + &.hover, + [open] > & { + background-color: var(--color-btn-hover-bg); + border-color: var(--color-btn-hover-border); + transition-duration: 0.1s; + } + + &:active { + background-color: var(--color-btn-active-bg); + border-color: var(--color-btn-active-border); + transition: none; + } + + &.Button--pressed, + &[aria-pressed='true'] { + background-color: var(--color-btn-selected-bg); + box-shadow: var(--color-primer-shadow-inset); + } + + &:disabled, + &.disabled, + &[aria-disabled='true'] { + color: var(--color-primer-fg-disabled); + background-color: var(--color-btn-bg); + border-color: var(--color-btn-border); + fill: var(--color-primer-fg-disabled); + } + + &.Button-small { + height: 28px; + font-size: $font-size-small; + } + + &.Button-large { + } + + &.Button-primary { + color: var(--color-btn-primary-text); + background-color: var(--color-btn-primary-bg); + border-color: var(--color-btn-primary-border); + box-shadow: var(--color-btn-primary-shadow), var(--color-btn-primary-inset-shadow); + + &:hover, + &.hover, + [open] > & { + background-color: var(--color-btn-primary-hover-bg); + border-color: var(--color-btn-primary-hover-border); + } + + &:active, + &.Button--pressed, + &[aria-pressed='true'] { + background-color: var(--color-btn-primary-selected-bg); + box-shadow: var(--color-btn-primary-selected-shadow); + } + + &:disabled, + &.disabled, + &[aria-disabled='true'] { + color: var(--color-btn-primary-disabled-text); + background-color: var(--color-btn-primary-disabled-bg); + border-color: var(--color-btn-primary-disabled-border); + fill: var(--color-btn-primary-disabled-text); + } + } + + &.Button-secondary { + } + + &.Button-outline { + color: var(--color-btn-outline-text); + + &:hover, + [open] > & { + color: var(--color-btn-outline-hover-text); + background-color: var(--color-btn-outline-hover-bg); + border-color: var(--color-btn-outline-hover-border); + box-shadow: var(--color-btn-outline-hover-shadow), var(--color-btn-outline-hover-inset-shadow); + } + + &:active, + &.Button--pressed, + &[aria-pressed='true'] { + color: var(--color-btn-outline-selected-text); + background-color: var(--color-btn-outline-selected-bg); + border-color: var(--color-btn-outline-selected-border); + box-shadow: var(--color-btn-outline-selected-shadow); + } + + &:disabled, + &.disabled, + &[aria-disabled='true'] { + color: var(--color-btn-outline-disabled-text); + background-color: var(--color-btn-outline-disabled-bg); + border-color: var(--color-btn-border); + box-shadow: none; + } + } + + &.Button-danger { + color: var(--color-btn-danger-text); + + .octicon { + color: var(--color-btn-danger-icon); + } + + &:hover, + [open] > & { + color: var(--color-btn-danger-hover-text); + background-color: var(--color-btn-danger-hover-bg); + border-color: var(--color-btn-danger-hover-border); + box-shadow: var(--color-btn-danger-hover-shadow), var(--color-btn-danger-hover-inset-shadow); + } + + &:active, + &.Button--pressed, + &[aria-pressed='true'] { + color: var(--color-btn-danger-selected-text); + background-color: var(--color-btn-danger-selected-bg); + border-color: var(--color-btn-danger-selected-border); + box-shadow: var(--color-btn-danger-selected-shadow); + } + + &:disabled, + &.disabled, + &[aria-disabled='true'] { + color: var(--color-btn-danger-disabled-text); + background-color: var(--color-btn-danger-disabled-bg); + border-color: var(--color-btn-border); + box-shadow: none; + } + } +} + +.Button--label { + grid-area: 'text'; + white-space: nowrap; +} + +.Button--visual { + display: flex; +} + +.Button--leadingVisual { + grid-area: 'leadingVisual'; +} + +.Button--trailingVisual { + grid-area: 'trailingVisual'; +} + +.Button--trailingAction { + grid-area: 'leadingAction'; +} diff --git a/src/button-refactor/link.scss b/src/button-refactor/link.scss new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/core/index.scss b/src/core/index.scss index 166da98b06..513f570ee4 100644 --- a/src/core/index.scss +++ b/src/core/index.scss @@ -24,6 +24,6 @@ @import '../pagination/index.scss'; @import '../tooltips/index.scss'; @import '../truncate/index.scss'; - +@import '../button-refactor/button.scss'; // Utilities always go last so that they can override components @import '../utilities/index.scss'; From 14e9185b67642170363b5d2efc4a238f36a9f707 Mon Sep 17 00:00:00 2001 From: langermank Date: Mon, 17 Jan 2022 20:17:48 -0800 Subject: [PATCH 03/16] more config --- .../stories/explorations/Button.stories.jsx | 104 +++++++---- .../explorations/IconButton.stories.jsx | 174 ++++++++++++++++++ .../src/stories/explorations/Link.stories.jsx | 131 +++++++++++++ src/button-refactor/ButtonsLinksAPI.md | 61 +++++- .../button-styled-as-link.scss | 0 src/button-refactor/button.scss | 67 ++++--- src/button-refactor/icon-button.scss | 10 + .../link-styled-as-button.scss | 0 src/button-refactor/link.scss | 61 ++++++ src/core/index.scss | 4 + 10 files changed, 556 insertions(+), 56 deletions(-) create mode 100644 docs/src/stories/explorations/IconButton.stories.jsx create mode 100644 docs/src/stories/explorations/Link.stories.jsx create mode 100644 src/button-refactor/button-styled-as-link.scss create mode 100644 src/button-refactor/icon-button.scss create mode 100644 src/button-refactor/link-styled-as-button.scss diff --git a/docs/src/stories/explorations/Button.stories.jsx b/docs/src/stories/explorations/Button.stories.jsx index d9ecc7e234..33979e080f 100644 --- a/docs/src/stories/explorations/Button.stories.jsx +++ b/docs/src/stories/explorations/Button.stories.jsx @@ -15,45 +15,73 @@ export default { argTypes: { variant: { options: [0, 1, 2, 3], // iterator - mapping: [null, 'Button-primary', 'Button-outline', 'Button-danger'], // values + mapping: ['Button--default', 'Button--primary', 'Button--outline', 'Button--danger'], // values control: { - type: 'select', + type: 'inline-radio', labels: ['default', 'primary', 'outline', 'danger'] }, table: { category: 'CSS' - } + }, + description: 'Controls button color', + defaultValue: 0 }, size: { options: [0, 1, 2], // iterator - mapping: [null, 'Button-small', 'Button-large'], // values + mapping: [null, 'Button--small', 'Button--large'], // values control: { - type: 'select', - labels: ['default', 'small', 'large'] + type: 'inline-radio', + labels: ['default [32px]', 'small [28px]', 'large'] }, table: { category: 'CSS' - } + }, + description: 'Controls button height', + defaultValue: 0 + }, + visualPosition: { + options: [0, 1], // iterator + mapping: [null, 'Button-visual--fixed'], // values + control: { + type: 'inline-radio', + labels: ['default', 'fixed'] + }, + table: { + category: 'CSS' + }, + description: + '[Name TBD!] Controls where the leading or trailing visuals position themselves in a fullWidth button (lock to text label or button bounds)', + defaultValue: 'Button-visual--fixed' }, label: { defaultValue: 'Button', type: 'string', name: 'label', - description: 'string', + description: 'Visible button label', table: { - category: 'HTML' + category: 'Slot' + } + }, + ariaLabel: { + defaultValue: '', + type: 'string', + name: 'ariaLabel', + description: 'Hidden button label (in addition to visible label). Not required in all cases.', + table: { + category: 'Slot' } }, disabled: { defaultValue: false, control: {type: 'boolean'}, table: { - category: 'Interactive' + category: 'State' } }, fullWidth: { - defaultValue: false, + defaultValue: true, control: {type: 'boolean'}, + description: 'Allow button to stretch and fill container', table: { category: 'CSS' } @@ -61,45 +89,49 @@ export default { leadingVisual: { name: 'leadingVisual', control: {type: 'boolean'}, - description: 'Paste [Octicon](https://primer.style/octicons/) in control field', + description: 'Slot for SVG icon or emoji (boolean only for testing purposes)', + defaultValue: false, table: { - category: 'HTML' + category: 'Slot' } }, trailingVisual: { name: 'trailingVisual', control: {type: 'boolean'}, - description: 'Paste [Octicon](https://primer.style/octicons/) in control field', + description: 'Slot for SVG icon or emoji (boolean only for testing purposes)', table: { - category: 'HTML' - } + category: 'Slot' + }, + defaultValue: false }, trailingAction: { defaultValue: false, control: {type: 'boolean'}, + description: + 'Slot for SVG icon that indicates an action. Primarily used by other Primer components, like a DropdownMenu or overlay trigger (boolean only for testing purposes)', table: { - category: 'CSS' + category: 'Slot' } }, pressed: { defaultValue: false, control: {type: 'boolean'}, table: { - category: 'CSS' + category: 'State' } }, focusElement: { control: {type: 'boolean'}, description: 'set focus on one element', table: { - category: 'Interactive' + category: 'State' } }, focusAllElements: { control: {type: 'boolean'}, description: 'set focus on all elements for viewing in all themes', table: { - category: 'Interactive' + category: 'State' } } } @@ -112,11 +144,11 @@ const focusMethod = function getFocus() { button.focus() } -const mona = ( +const star = ( ) @@ -138,25 +170,30 @@ export const ButtonTemplate = ({ trailingAction, pressed, focusElement, - focusAllElements + focusAllElements, + visualPosition, + className }) => ( <> {focusElement && focusMethod()} @@ -166,7 +203,8 @@ export const Playground = ButtonTemplate.bind({}) Playground.args = { focusElement: false, focusAllElements: false, - leadingVisual: false, - trailingAction: false, - trailingVisual: false + variant: 'Button--default' + // leadingVisual: false, + // trailingAction: false, + // trailingVisual: false } diff --git a/docs/src/stories/explorations/IconButton.stories.jsx b/docs/src/stories/explorations/IconButton.stories.jsx new file mode 100644 index 0000000000..3327d1f12b --- /dev/null +++ b/docs/src/stories/explorations/IconButton.stories.jsx @@ -0,0 +1,174 @@ +import React from 'react' +import clsx from 'clsx' + +export default { + title: 'Explorations/IconButton', + parameters: { + // design: { + // type: 'figma', + // url: 'https://www.figma.com/file/GCvY3Qv8czRgZgvl1dG6lp/Primer-Web?node-id=4371%3A7128' + // }, + layout: 'padded' + }, + + excludeStories: ['IconButtonTemplate'], + argTypes: { + variant: { + options: [0, 1, 2, 3], // iterator + mapping: ['Button--default', 'Button--primary', 'Button--outline', 'Button--danger'], // values + control: { + type: 'inline-radio', + labels: ['default', 'primary', 'outline', 'danger'] + }, + table: { + category: 'CSS' + }, + description: 'Controls button color', + defaultValue: 0 + }, + size: { + options: [0, 1, 2], // iterator + mapping: [null, 'Button--small', 'Button--large'], // values + control: { + type: 'inline-radio', + labels: ['default [32px]', 'small [28px]', 'large'] + }, + table: { + category: 'CSS' + }, + description: 'Controls button height', + defaultValue: 0 + }, + visualPosition: { + options: [0, 1], // iterator + mapping: [null, 'Button-visual--fixed'], // values + control: { + type: 'inline-radio', + labels: ['default', 'fixed'] + }, + table: { + category: 'CSS' + }, + description: + '[Name TBD!] Controls where the leading or trailing visuals position themselves in a fullWidth button (lock to text label or button bounds)', + defaultValue: 'Button-visual--fixed' + }, + ariaLabel: { + defaultValue: '', + type: 'string', + name: 'ariaLabel', + description: 'Hidden button label (required)', + table: { + category: 'Slot' + } + }, + disabled: { + defaultValue: false, + control: {type: 'boolean'}, + table: { + category: 'State' + } + }, + fullWidth: { + defaultValue: true, + control: {type: 'boolean'}, + description: 'Allow button to stretch and fill container', + table: { + category: 'CSS' + } + }, + visual: { + name: 'visual', + control: {type: 'boolean'}, + description: 'Slot for SVG icon (boolean only for testing purposes)', + defaultValue: true, + table: { + category: 'Slot' + } + }, + pressed: { + defaultValue: false, + control: {type: 'boolean'}, + table: { + category: 'State' + } + }, + focusElement: { + control: {type: 'boolean'}, + description: 'set focus on one element', + table: { + category: 'State' + } + }, + focusAllElements: { + control: {type: 'boolean'}, + description: 'set focus on all elements for viewing in all themes', + table: { + category: 'State' + } + } + } +} + +const focusMethod = function getFocus() { + // find the focusable element + var button = document.getElementsByTagName('button')[0] + // set focus on element + button.focus() +} + +const star = ( + + + +) + +const caret = ( + + + +) + +export const IconButtonTemplate = ({ + variant, + disabled, + size, + visual, + pressed, + focusElement, + focusAllElements, + className, + ariaLabel +}) => ( + <> + + {focusElement && focusMethod()} + +) + +export const Playground = IconButtonTemplate.bind({}) +Playground.args = { + focusElement: false, + focusAllElements: false, + ariaLabel: 'Button description' + // leadingVisual: false, + // trailingAction: false, + // trailingVisual: false +} diff --git a/docs/src/stories/explorations/Link.stories.jsx b/docs/src/stories/explorations/Link.stories.jsx new file mode 100644 index 0000000000..42fdc5236f --- /dev/null +++ b/docs/src/stories/explorations/Link.stories.jsx @@ -0,0 +1,131 @@ +import React from 'react' +import clsx from 'clsx' + +export default { + title: 'Explorations/Link', + parameters: { + // design: { + // type: 'figma', + // url: 'https://www.figma.com/file/GCvY3Qv8czRgZgvl1dG6lp/Primer-Web?node-id=4371%3A7128' + // }, + layout: 'padded' + }, + + excludeStories: ['LinkTemplate'], + argTypes: { + variant: { + options: [0, 1, 2, 3], // iterator + mapping: ['Button--default', 'Button--primary', 'Button--outline', 'Button--danger'], // values + control: { + type: 'inline-radio', + labels: ['default', 'primary', 'outline', 'danger'] + }, + table: { + category: 'CSS' + }, + description: 'Controls link color', + defaultValue: 0 + }, + label: { + defaultValue: 'Link', + type: 'string', + name: 'label', + description: 'Link label', + table: { + category: 'Slot' + } + }, + target: { + defaultValue: '', + type: 'string', + name: 'target', + description: '_blank for new tab', + table: { + category: 'Slot' + } + }, + href: { + defaultValue: '', + type: 'string', + name: 'href', + description: '', + table: { + category: 'Slot' + } + }, + showTrailingVisual: { + defaultValue: false, + control: {type: 'boolean'}, + description: '', + table: { + category: 'CSS' + } + }, + focusElement: { + control: {type: 'boolean'}, + description: 'set focus on one element', + table: { + category: 'State' + } + }, + focusAllElements: { + control: {type: 'boolean'}, + description: 'set focus on all elements for viewing in all themes', + table: { + category: 'State' + } + } + } +} + +const focusMethod = function getFocus() { + // find the focusable element + var button = document.getElementsByTagName('button')[0] + // set focus on element + button.focus() +} + +const arrow = ( + + + +) + +export const LinkTemplate = ({ + label, + variant, + focusElement, + focusAllElements, + className, + href, + showTrailingVisual, + target +}) => ( + <> + + {showTrailingVisual ? ( + + {label} + {arrow} + + ) : ( + label + )} + + {focusElement && focusMethod()} + +) + +export const Playground = LinkTemplate.bind({}) +Playground.args = { + focusElement: false, + focusAllElements: false + // variant: 'Button--default' +} diff --git a/src/button-refactor/ButtonsLinksAPI.md b/src/button-refactor/ButtonsLinksAPI.md index 7150d13046..e8e9a85a35 100644 --- a/src/button-refactor/ButtonsLinksAPI.md +++ b/src/button-refactor/ButtonsLinksAPI.md @@ -65,4 +65,63 @@ A few discussions were about naming and prop drilling (source here). We found th | `fullWidth` | boolean | `true/false` | `false` | | | `visualPosition` | one-of string | `fixed` `some-other-word` | `fixed` | lock icon to the text label or to the button container | -## Component design specs + +### LinkStyledAsButton + +| prop | type | options | default | notes | +| -- | -- | -- | -- | -- | +| `variant` | one-of string | `primary` `secondary` `danger` `outline`? | `secondary` | | +| `size` | one-of string | `small` `default` `large` | `default | | +| `label` | string | button description | null | | +| `leadingVisual` | children (slot) | octicon | null | | +| `trailingVisual` | children (slot) | octicon | null | | +| `trailingAction` | children (slot) | octicon | null | slot for caret to maintain leading/trailing visuals if they exist | +| `fullWidth` | boolean | `true/false` | `false` | | +| `visualPosition` | one-of string | `fixed` `some-other-word` | `fixed` | lock icon to the text label or to the button container | + +### IconButton + +| prop | type | options | default | notes | +| -- | -- | -- | -- | -- | +| `variant` | one-of string | `primary` `secondary` `danger` `outline`? | `secondary` | | +| `size` | one-of string | `small` `default` `large` | `default | | +| `aria-label` | string | button description for screen readers (required) | null | | +| `visual` | children (slot) | octicon | null | | +| `aria-pressed` | boolean | `true/false` | `false` | | + +### Link + +| prop | type | options | default | notes | +| -- | -- | -- | -- | -- | +| `variant` | one-of string | ?? we need to work on this (subtle and or muted) | | +| `label` | string | button description | null | | +| `showTrailingVisual` | boolean | specific octicon for new tab or new page (or anchor?) | null | | + +### ButtonStyledAsLink + +| prop | type | options | default | notes | +| -- | -- | -- | -- | -- | +| `variant` | one-of string | see above confusion | `secondary` | | +| `size` | one-of string | `small` `default` `large` | `default | | +| `label` | string | button description | null | | +| `aria-label` | string | button description for screen readers | null | | +| `aria-pressed` | boolean | `true/false` | `false` | | +| `leadingVisual` | children (slot) | octicon | null | | +| `trailingVisual` | children (slot) | octicon | null | | +| `fullWidth` | boolean | `true/false` | `false` | | +| `visualPosition` | one-of string | `fixed` `some-other-word` | `fixed` | I think this should be fixed for link styles (so maybe we dont need this prop) | + +### ButtonGroup + +| prop | type | options | default | notes | +| -- | -- | -- | -- | -- | +| | | | | + +## Notes +[] Should Button have a `secondary` variant, or should it be the default with no additional class? +[] How granular should icon positioning be? +- If both trailingVisual and trailingAction exist, and visualPosition is `fixed`, the trailingVisual will "lock" to the trailingAction (unless we are more granular and specify different lock scenarios as props) +- One idea would be to always lock trailingAction (always affix it to the right of a button which will only be visible on full width) and maintain visualPosition prop _only_ for visuals +[] Thoughts on buttons with trailingAction having less margin-right? +[] Should we think about underlining links with this work, or is that scope creep? +[] Button styled as link is trickiest, and we need to decide how much logic that component can have compared to Button diff --git a/src/button-refactor/button-styled-as-link.scss b/src/button-refactor/button-styled-as-link.scss new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/button-refactor/button.scss b/src/button-refactor/button.scss index 4251fdbd97..34ece55a91 100644 --- a/src/button-refactor/button.scss +++ b/src/button-refactor/button.scss @@ -4,20 +4,22 @@ font-weight: $font-weight-semibold; cursor: pointer; user-select: none; + background-color: transparent; border: $border-width $border-style; + border-color: transparent; border-radius: $border-radius; color: var(--color-btn-text); - background-color: var(--color-btn-bg); - border-color: var(--color-btn-border); - box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); transition: 0.2s cubic-bezier(0.3, 0, 0.5, 1); transition-property: color, background-color, border-color; height: 32px; padding: 0 16px; - display: grid; - grid-template-areas: 'leadingIcon text trailingIcon trailingAction'; - grid-template-columns: min-content minmax(0, auto) min-content min-content; - align-items: center; + + &:not(.Button--iconOnly) { + display: grid; + grid-template-areas: 'leadingVisual text trailingVisual trailingAction'; + grid-template-columns: min-content minmax(0, auto) min-content min-content; + align-items: center; + } // column-gap persists with empty grid-areas, margin applies only when children exist > :not(:last-child) { @@ -53,15 +55,31 @@ fill: var(--color-primer-fg-disabled); } - &.Button-small { + &.Button--small { height: 28px; font-size: $font-size-small; } - &.Button-large { + &.Button--large { + } + + &.Button--fullWidth { + width: 100%; + + .Button-trailingVisual { + margin-left: auto; + } + } + + &.Button--default { + color: var(--color-btn-text); + background-color: var(--color-btn-bg); + border: $border-width $border-style; + border-color: var(--color-btn-border); + box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); } - &.Button-primary { + &.Button--primary { color: var(--color-btn-primary-text); background-color: var(--color-btn-primary-bg); border-color: var(--color-btn-primary-border); @@ -91,10 +109,10 @@ } } - &.Button-secondary { + &.Button--secondary { } - &.Button-outline { + &.Button--outline { color: var(--color-btn-outline-text); &:hover, @@ -124,7 +142,7 @@ } } - &.Button-danger { + &.Button--danger { color: var(--color-btn-danger-text); .octicon { @@ -159,23 +177,28 @@ } } -.Button--label { - grid-area: 'text'; +.Button-label { + grid-area: text; white-space: nowrap; } -.Button--visual { +.Button-visual { display: flex; } -.Button--leadingVisual { - grid-area: 'leadingVisual'; +.Button-visual--fixed { + grid-template-columns: min-content min-content min-content min-content; + place-content: center; +} + +.Button-leadingVisual { + grid-area: leadingVisual; } -.Button--trailingVisual { - grid-area: 'trailingVisual'; +.Button-trailingVisual { + grid-area: trailingVisual; } -.Button--trailingAction { - grid-area: 'leadingAction'; +.Button-trailingAction { + grid-area: trailingAction; } diff --git a/src/button-refactor/icon-button.scss b/src/button-refactor/icon-button.scss new file mode 100644 index 0000000000..d2e849689c --- /dev/null +++ b/src/button-refactor/icon-button.scss @@ -0,0 +1,10 @@ +.Button--iconOnly { + display: grid; + place-content: center; + padding: unset; + width: 32px; + + &.Button--small { + width: 28px; + } +} diff --git a/src/button-refactor/link-styled-as-button.scss b/src/button-refactor/link-styled-as-button.scss new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/button-refactor/link.scss b/src/button-refactor/link.scss index e69de29bb2..1cd33bf5c5 100644 --- a/src/button-refactor/link.scss +++ b/src/button-refactor/link.scss @@ -0,0 +1,61 @@ +// Links + +.Link { + color: var(--color-accent-fg); + + &:hover { + text-decoration: underline; + cursor: pointer; + } +} + +.Link--trailingVisual { + display: flex; + flex-direction: row; + align-items: center; + gap: 4px; +} +// .Link--primary { +// color: var(--color-fg-default) !important; + +// &:hover { +// color: var(--color-accent-fg) !important; +// } +// } + +// .Link--secondary { +// color: var(--color-fg-muted) !important; + +// &:hover { +// color: var(--color-accent-fg) !important; +// } +// } + +// .Link--muted { +// color: var(--color-fg-muted) !important; + +// &:hover { +// color: var(--color-accent-fg) !important; +// text-decoration: none; +// } +// } + +// // Set the link color only on hover +// // Useful when you want only part of a link to turn blue on hover +// .Link--onHover { +// &:hover { +// color: var(--color-accent-fg) !important; +// text-decoration: underline; +// cursor: pointer; +// } +// } + +// // When using a color utility class inside of a link class, +// // color should change with link on hover. +// .Link--secondary, +// .Link--primary, +// .Link--muted { +// &:hover [class*='color-text'] { +// color: inherit !important; +// } +// } diff --git a/src/core/index.scss b/src/core/index.scss index 513f570ee4..611d9ddb55 100644 --- a/src/core/index.scss +++ b/src/core/index.scss @@ -25,5 +25,9 @@ @import '../tooltips/index.scss'; @import '../truncate/index.scss'; @import '../button-refactor/button.scss'; +@import '../button-refactor/link.scss'; +@import '../button-refactor/icon-button.scss'; +@import '../button-refactor/button-styled-as-link.scss'; +@import '../button-refactor/link-styled-as-button.scss'; // Utilities always go last so that they can override components @import '../utilities/index.scss'; From 1c67f020742f0be026ffbd071ffb8719d9bfacac Mon Sep 17 00:00:00 2001 From: langermank Date: Tue, 18 Jan 2022 13:10:53 -0800 Subject: [PATCH 04/16] stash meeting notes --- .../src/stories/explorations/Button.stories.jsx | 3 ++- src/button-refactor/ButtonsLinksAPI.md | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/docs/src/stories/explorations/Button.stories.jsx b/docs/src/stories/explorations/Button.stories.jsx index 33979e080f..e92d9ccf31 100644 --- a/docs/src/stories/explorations/Button.stories.jsx +++ b/docs/src/stories/explorations/Button.stories.jsx @@ -172,7 +172,8 @@ export const ButtonTemplate = ({ focusElement, focusAllElements, visualPosition, - className + className, + ariaLabel }) => ( <> {focusElement && focusMethod()} diff --git a/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx b/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx index 9413b1c112..3387d1244a 100644 --- a/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx +++ b/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx @@ -46,7 +46,7 @@ export default { }, visualPosition: { options: [0, 1], // iterator - mapping: [null, 'Button-visual--fixed'], // values + mapping: [null, 'LinkStyledAsButton-content--visualFixed'], // values control: { type: 'inline-radio', labels: ['default', 'fixed'] @@ -201,14 +201,15 @@ export const LinkStyledAsButtonTemplate = ({ variant && `${variant}`, size && `${size}`, fullWidth && 'LinkStyledAsButton--fullWidth', - visualPosition && `${visualPosition}`, active && 'LinkStyledAsButton--active' )} > {/* {leadingVisual && } */} - {leadingVisual && {star}} - {label} - {trailingVisual && {star}} + + {leadingVisual && {star}} + {label} + {trailingVisual && {star}} + {showTrailingAction && ( {arrow} )} diff --git a/src/button-refactor/ButtonsLinksAPI.md b/src/button-refactor/ButtonsLinksAPI.md index 0cabed584f..edd6ba45e5 100644 --- a/src/button-refactor/ButtonsLinksAPI.md +++ b/src/button-refactor/ButtonsLinksAPI.md @@ -108,3 +108,4 @@ A few discussions were about naming and prop drilling (source here). We found th - [ ] for `LinkStyledAsButton` should `trailingAction` slot be reserved for the > chevron indicating this is a link and not a button? - [ ] What color should `invisible` button variant be? Blue or grey? - [ ] No aria-label for `LinkStyledAsButton` +- [ ] Do we like `fullWidth` or `block` as a prop for width behavior? diff --git a/src/button-refactor/button.scss b/src/button-refactor/button.scss index c59cf7e0d0..02a865f96e 100644 --- a/src/button-refactor/button.scss +++ b/src/button-refactor/button.scss @@ -16,18 +16,13 @@ height: 32px; padding: 0 16px; - // define grid slots for standard buttons - &:not(.Button--iconOnly) { - display: grid; - grid-template-areas: 'leadingVisual text trailingVisual trailingAction'; - grid-template-columns: min-content minmax(0, auto) min-content min-content; - align-items: center; - - // column-gap persists with empty grid-areas, margin applies only when children exist - > :not(:last-child) { - margin-right: $spacer-2; - } - } + // define grid slots and layout for standard button contents + // &:not(.Button--iconOnly) { + display: flex; + flex-direction: row; + justify-content: space-between; + align-items: center; + gap: $spacer-2; // base states @@ -51,191 +46,200 @@ &:focus { @include focusOutline; } +} - // sizes +// wrap grid content to allow trailingAction to lock-right +.Button-content { + flex: 1 0 auto; + display: grid; + grid-template-areas: 'leadingVisual text trailingVisual'; + grid-template-columns: min-content minmax(0, auto) min-content; + align-items: center; - &.Button--small { - height: 28px; - font-size: $font-size-small; + > :not(:last-child) { + margin-right: $spacer-2; } +} - &.Button--large { - height: 44px; - } +// center child elements for fullWidth +.Button-content--visualFixed { + place-content: center; +} + +// button child elements + +// align svg +.Button-visual { + display: flex; +} - &.Button--fullWidth { - width: 100%; +.Button-label { + grid-area: text; + white-space: nowrap; +} + +.Button-leadingVisual { + grid-area: leadingVisual; +} + +.Button-trailingVisual { + grid-area: trailingVisual; +} - .Button-trailingVisual { - margin-left: auto; - } +.Button-trailingAction { + margin-right: -8px; +} + +// sizes + +.Button--small { + height: 28px; + font-size: $font-size-small; +} + +.Button--large { + height: 44px; +} + +.Button--fullWidth { + width: 100%; +} + +// variants + +// primary +.Button--primary { + color: var(--color-btn-primary-text); + fill: var(--color-btn-primary-icon); + background-color: var(--color-btn-primary-bg); + border-color: var(--color-btn-primary-border); + box-shadow: var(--color-btn-primary-shadow), var(--color-btn-primary-inset-shadow); + + &:hover, + [open] > & { + background-color: var(--color-btn-primary-hover-bg); + border-color: var(--color-btn-primary-hover-border); } - &.Button-visual--fixed { - grid-template-columns: min-content min-content min-content min-content; - place-content: center; + &:active, + &[aria-pressed='true'], + &.Button--pressed { + background-color: var(--color-btn-primary-selected-bg); + box-shadow: var(--color-btn-primary-selected-shadow); } - // variants + &:disabled, + &.Button--disabled, + &[aria-disabled='true'] { + color: var(--color-btn-primary-disabled-text); + background-color: var(--color-btn-primary-disabled-bg); + border-color: var(--color-btn-primary-disabled-border); + fill: var(--color-btn-primary-disabled-text); + } +} - // primary - &.Button--primary { - color: var(--color-btn-primary-text); - fill: var(--color-btn-primary-icon); - background-color: var(--color-btn-primary-bg); - border-color: var(--color-btn-primary-border); - box-shadow: var(--color-btn-primary-shadow), var(--color-btn-primary-inset-shadow); +// default (secondary) +.Button--secondary { + color: var(--color-btn-text); + fill: var(--color-fg-muted); // help this + background-color: var(--color-btn-bg); + border: $border-width $border-style; + border-color: var(--color-btn-border); + box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); - &:hover, - [open] > & { - background-color: var(--color-btn-primary-hover-bg); - border-color: var(--color-btn-primary-hover-border); - } + &:hover, + [open] > & { + background-color: var(--color-btn-hover-bg); + border-color: var(--color-btn-hover-border); + } - &:active, - &[aria-pressed='true'], - &.Button--pressed { - background-color: var(--color-btn-primary-selected-bg); - box-shadow: var(--color-btn-primary-selected-shadow); - } + &:active, + &.Button--active { + background-color: var(--color-btn-active-bg); + border-color: var(--color-btn-active-border); + } - &:disabled, - &.Button--disabled, - &[aria-disabled='true'] { - color: var(--color-btn-primary-disabled-text); - background-color: var(--color-btn-primary-disabled-bg); - border-color: var(--color-btn-primary-disabled-border); - fill: var(--color-btn-primary-disabled-text); - } + &[aria-pressed='true'], + &.Button--pressed { + background-color: var(--color-btn-selected-bg); + box-shadow: var(--color-primer-shadow-inset); } - // default (secondary) - &.Button--secondary { - color: var(--color-btn-text); - fill: var(--color-fg-muted); // help this - background-color: var(--color-btn-bg); - border: $border-width $border-style; - border-color: var(--color-btn-border); - box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); - - &:hover, - [open] > & { - background-color: var(--color-btn-hover-bg); - border-color: var(--color-btn-hover-border); - } - - &:active, - &.Button--active { - background-color: var(--color-btn-active-bg); - border-color: var(--color-btn-active-border); - } - - &[aria-pressed='true'], - &.Button--pressed { - background-color: var(--color-btn-selected-bg); - box-shadow: var(--color-primer-shadow-inset); - } - - &:disabled, - &.Button--disabled, - &[aria-disabled='true'] { - color: var(--color-primer-fg-disabled); - background-color: var(--color-btn-bg); - border-color: var(--color-btn-border); - fill: var(--color-primer-fg-disabled); - } - } - - &.Button--invisible { - color: var(--color-btn-outline-text); - fill: var(--color-btn-outline-text); - background-color: var(--color-canvas-default); - border: $border-width $border-style; - border-color: var(--color-canvas-default); - - &:hover, - [open] > & { - background-color: var(--color-btn-hover-bg); - border-color: var(--color-btn-hover-bg); - } - - &[aria-pressed='true'], - &:active, - &.Button--active, - &.Button--pressed { - background-color: var(--color-btn-selected-bg); - box-shadow: var(--color-primer-shadow-inset); - } - - &:disabled, - &.Button--disabled, - &[aria-disabled='true'] { - color: var(--color-primer-fg-disabled); - background-color: var(--color-btn-bg); - border-color: var(--color-btn-border); - fill: var(--color-primer-fg-disabled); - } - } - - // danger - &.Button--danger { - color: var(--color-btn-danger-text); - fill: var(--color-btn-danger-icon); + &:disabled, + &.Button--disabled, + &[aria-disabled='true'] { + color: var(--color-primer-fg-disabled); background-color: var(--color-btn-bg); - border: $border-width $border-style; border-color: var(--color-btn-border); - box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); - - &:hover, - [open] > & { - color: var(--color-btn-danger-hover-text); - fill: var(--color-btn-danger-hover-text); - background-color: var(--color-btn-danger-hover-bg); - border-color: var(--color-btn-danger-hover-border); - box-shadow: var(--color-btn-danger-hover-shadow), var(--color-btn-danger-hover-inset-shadow); - } - - &:active, - &[aria-pressed='true'], - &.Button--pressed { - color: var(--color-btn-danger-selected-text); - fill: var(--color-btn-danger-selected-text); - background-color: var(--color-btn-danger-selected-bg); - border-color: var(--color-btn-danger-selected-border); - box-shadow: var(--color-btn-danger-selected-shadow); - } - - &:disabled, - &.disabled, - &[aria-disabled='true'] { - color: var(--color-btn-danger-disabled-text); - fill: var(--color-btn-danger-disabled-text); - background-color: var(--color-btn-danger-disabled-bg); - border-color: var(--color-btn-border); - } + fill: var(--color-primer-fg-disabled); } } -// button child elements +.Button--invisible { + color: var(--color-btn-outline-text); + fill: var(--color-btn-outline-text); + background-color: var(--color-canvas-default); + border: $border-width $border-style; + border-color: var(--color-canvas-default); -.Button-label { - grid-area: text; - white-space: nowrap; -} + &:hover, + [open] > & { + background-color: var(--color-btn-hover-bg); + border-color: var(--color-btn-hover-bg); + } -.Button-leadingVisual { - grid-area: leadingVisual; -} + &[aria-pressed='true'], + &:active, + &.Button--active, + &.Button--pressed { + background-color: var(--color-btn-selected-bg); + box-shadow: var(--color-primer-shadow-inset); + } -.Button-trailingVisual { - grid-area: trailingVisual; + &:disabled, + &.Button--disabled, + &[aria-disabled='true'] { + color: var(--color-primer-fg-disabled); + background-color: var(--color-btn-bg); + border-color: var(--color-btn-border); + fill: var(--color-primer-fg-disabled); + } } -.Button-trailingAction { - grid-area: trailingAction; - margin-right: -8px; -} +// danger +.Button--danger { + color: var(--color-btn-danger-text); + fill: var(--color-btn-danger-icon); + background-color: var(--color-btn-bg); + border: $border-width $border-style; + border-color: var(--color-btn-border); + box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); -.Button-visual { - display: flex; + &:hover, + [open] > & { + color: var(--color-btn-danger-hover-text); + fill: var(--color-btn-danger-hover-text); + background-color: var(--color-btn-danger-hover-bg); + border-color: var(--color-btn-danger-hover-border); + box-shadow: var(--color-btn-danger-hover-shadow), var(--color-btn-danger-hover-inset-shadow); + } + + &:active, + &[aria-pressed='true'], + &.Button--pressed { + color: var(--color-btn-danger-selected-text); + fill: var(--color-btn-danger-selected-text); + background-color: var(--color-btn-danger-selected-bg); + border-color: var(--color-btn-danger-selected-border); + box-shadow: var(--color-btn-danger-selected-shadow); + } + + &:disabled, + &.disabled, + &[aria-disabled='true'] { + color: var(--color-btn-danger-disabled-text); + fill: var(--color-btn-danger-disabled-text); + background-color: var(--color-btn-danger-disabled-bg); + border-color: var(--color-btn-border); + } } diff --git a/src/button-refactor/icon-button.scss b/src/button-refactor/icon-button.scss index d2e849689c..001f59f2dc 100644 --- a/src/button-refactor/icon-button.scss +++ b/src/button-refactor/icon-button.scss @@ -7,4 +7,8 @@ &.Button--small { width: 28px; } + + &.Button--large { + width: 44px; + } } diff --git a/src/button-refactor/link-styled-as-button.scss b/src/button-refactor/link-styled-as-button.scss index 85ee4d44bf..153bf405b2 100644 --- a/src/button-refactor/link-styled-as-button.scss +++ b/src/button-refactor/link-styled-as-button.scss @@ -1,235 +1,75 @@ -// base button +// base link .LinkStyledAsButton { - position: relative; - font-size: $body-font-size; - font-weight: $font-weight-semibold; - cursor: pointer; - user-select: none; - background-color: transparent; - border: $border-width $border-style; - border-color: transparent; - border-radius: $border-radius; - color: var(--color-btn-text); - transition: 0.2s cubic-bezier(0.3, 0, 0.5, 1); - transition-property: color, background-color, border-color; - text-align: center; - height: 32px; - padding: 0 16px; - - // define grid slots for standard buttons - &:not(.LinkStyledAsButton--iconOnly) { - display: inline-grid; - grid-template-areas: 'leadingVisual text trailingVisual trailingAction'; - grid-template-columns: min-content minmax(0, auto) min-content min-content; - align-items: center; - - // column-gap persists with empty grid-areas, margin applies only when children exist - > :not(:last-child) { - margin-right: $spacer-2; - } - } - - // base states + @extend .Button; + display: inline-flex; - &:hover, - [open] > & { - transition-duration: 0.1s; + &:hover { text-decoration: none; } +} - &:active, - &.LinkStyledAsButton--active { - transition: none; - } - - &:focus { - @include focusOutline; - } +.LinkStyledAsButton-content { + @extend .Button-content; +} - // sizes +.LinkStyledAsButton-content--visualFixed { + @extend .Button-content--visualFixed; +} - &.LinkStyledAsButton--small { - height: 28px; - font-size: $font-size-small; - } +// link child elements - &.LinkStyledAsButton--large { - height: 44px; - } +.LinkStyledAsButton-visual { + @extend .Button-visual; +} - &.LinkStyledAsButton--fullWidth { - width: 100%; +.LinkStyledAsButton-label { + @extend .Button-label; +} - .LinkStyledAsButton-trailingVisual { - margin-left: auto; - } - } +.LinkStyledAsButton-leadingVisual { + @extend .Button-leadingVisual; +} - // variants - - // primary - &.LinkStyledAsButton--primary { - color: var(--color-btn-primary-text); - fill: var(--color-btn-primary-icon); - background-color: var(--color-btn-primary-bg); - border-color: var(--color-btn-primary-border); - box-shadow: var(--color-btn-primary-shadow), var(--color-btn-primary-inset-shadow); - - &:hover, - [open] > & { - background-color: var(--color-btn-primary-hover-bg); - border-color: var(--color-btn-primary-hover-border); - } - - &:active, - &[aria-pressed='true'], - &.Button--pressed { - background-color: var(--color-btn-primary-selected-bg); - box-shadow: var(--color-btn-primary-selected-shadow); - } - - &:disabled, - &.Button--disabled, - &[aria-disabled='true'] { - color: var(--color-btn-primary-disabled-text); - background-color: var(--color-btn-primary-disabled-bg); - border-color: var(--color-btn-primary-disabled-border); - fill: var(--color-btn-primary-disabled-text); - } - } +.LinkStyledAsButton-trailingVisual { + @extend .Button-trailingVisual; +} - // default (secondary) - &.LinkStyledAsButton--secondary { - color: var(--color-btn-text); - fill: var(--color-fg-muted); // help this - background-color: var(--color-btn-bg); - border: $border-width $border-style; - border-color: var(--color-btn-border); - box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); - - &:hover, - [open] > & { - background-color: var(--color-btn-hover-bg); - border-color: var(--color-btn-hover-border); - } - - &:active, - &.Button--active { - background-color: var(--color-btn-active-bg); - border-color: var(--color-btn-active-border); - } - - &[aria-pressed='true'], - &.Button--pressed { - background-color: var(--color-btn-selected-bg); - box-shadow: var(--color-primer-shadow-inset); - } - - &:disabled, - &.Button--disabled, - &[aria-disabled='true'] { - color: var(--color-primer-fg-disabled); - background-color: var(--color-btn-bg); - border-color: var(--color-btn-border); - fill: var(--color-primer-fg-disabled); - } - } +.LinkStyledAsButton-trailingAction { + @extend .Button-trailingAction; +} - &.LinkStyledAsButton--invisible { - color: var(--color-btn-outline-text); - fill: var(--color-btn-outline-text); - background-color: var(--color-canvas-default); - border: $border-width $border-style; - border-color: var(--color-canvas-default); - - &:hover, - [open] > & { - background-color: var(--color-btn-hover-bg); - border-color: var(--color-btn-hover-bg); - } - - &[aria-pressed='true'], - &:active, - &.Button--active, - &.Button--pressed { - background-color: var(--color-btn-selected-bg); - box-shadow: var(--color-primer-shadow-inset); - } - - &:disabled, - &.Button--disabled, - &[aria-disabled='true'] { - color: var(--color-primer-fg-disabled); - background-color: var(--color-btn-bg); - border-color: var(--color-btn-border); - fill: var(--color-primer-fg-disabled); - } - } +// sizes - // danger - &.LinkStyledAsButton--danger { - color: var(--color-btn-danger-text); - fill: var(--color-btn-danger-icon); - background-color: var(--color-btn-bg); - border: $border-width $border-style; - border-color: var(--color-btn-border); - box-shadow: var(--color-btn-shadow), var(--color-btn-inset-shadow); - - &:hover, - [open] > & { - color: var(--color-btn-danger-hover-text); - fill: var(--color-btn-danger-hover-text); - background-color: var(--color-btn-danger-hover-bg); - border-color: var(--color-btn-danger-hover-border); - box-shadow: var(--color-btn-danger-hover-shadow), var(--color-btn-danger-hover-inset-shadow); - } - - &:active, - &[aria-pressed='true'], - &.Button--pressed { - color: var(--color-btn-danger-selected-text); - fill: var(--color-btn-danger-selected-text); - background-color: var(--color-btn-danger-selected-bg); - border-color: var(--color-btn-danger-selected-border); - box-shadow: var(--color-btn-danger-selected-shadow); - } - - &:disabled, - &.disabled, - &[aria-disabled='true'] { - color: var(--color-btn-danger-disabled-text); - fill: var(--color-btn-danger-disabled-text); - background-color: var(--color-btn-danger-disabled-bg); - border-color: var(--color-btn-border); - } - } +.LinkStyledAsButton--small { + @extend .Button--small; } -// button child elements - -.LinkStyledAsButton-label { - grid-area: text; - white-space: nowrap; +.LinkStyledAsButton--large { + @extend .Button--large; } -.LinkStyledAsButton-leadingVisual { - grid-area: leadingVisual; +.LinkStyledAsButton--fullWidth { + @extend .Button--fullWidth; } -.LinkStyledAsButton-trailingVisual { - grid-area: trailingVisual; +// variants + +// primary +.LinkStyledAsButton--primary { + @extend .Button--primary; } -.LinkStyledAsButton-trailingAction { - grid-area: trailingAction; - margin-right: -8px; +// default (secondary) +.LinkStyledAsButton--secondary { + @extend .Button--secondary; } -.LinkStyledAsButton-visual { - display: flex; +// invisible +.LinkStyledAsButton--invisible { + @extend .Button--invisible; } -.LinkStyledAsButton-visual--fixed { - grid-template-columns: min-content min-content min-content min-content; - place-content: center; +// danger +.LinkStyledAsButton--danger { + @extend .Button--danger; } From 3d882214340a96f5518b3f6df95cde77e26b5e51 Mon Sep 17 00:00:00 2001 From: langermank Date: Thu, 20 Jan 2022 11:32:14 -0800 Subject: [PATCH 07/16] button group --- .../explorations/ButtonGroup.stories.jsx | 24 +++++++++++++++++ src/button-refactor/ButtonsLinksAPI.md | 9 ++++--- src/button-refactor/button-group.scss | 27 +++++++++++++++++++ 3 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 docs/src/stories/explorations/ButtonGroup.stories.jsx diff --git a/docs/src/stories/explorations/ButtonGroup.stories.jsx b/docs/src/stories/explorations/ButtonGroup.stories.jsx new file mode 100644 index 0000000000..b91ae511cd --- /dev/null +++ b/docs/src/stories/explorations/ButtonGroup.stories.jsx @@ -0,0 +1,24 @@ +import React from 'react' +import clsx from 'clsx' +import {ButtonTemplate} from './Button.stories' +import {IconButtonTemplate} from './IconButton.stories' + +export default { + title: 'Explorations/ButtonGroup', + excludeStories: ['ButtonGroupTemplate'], + layout: 'padded', + argTypes: {} +} + +// build every component case here in the template (private api) +export const ButtonGroupTemplate = ({}) => ( +
+ + + + +
+) + +export const Playground = ButtonGroupTemplate.bind({}) +Playground.args = {} diff --git a/src/button-refactor/ButtonsLinksAPI.md b/src/button-refactor/ButtonsLinksAPI.md index edd6ba45e5..fbe1dc04e6 100644 --- a/src/button-refactor/ButtonsLinksAPI.md +++ b/src/button-refactor/ButtonsLinksAPI.md @@ -54,7 +54,7 @@ A few discussions were about naming and prop drilling (source here). We found th | prop | type | options | default | notes | | -- | -- | -- | -- | -- | | `variant` | one-of string | `primary` `secondary` `danger` `invisible` | `secondary` | | -| `size` | one-of string | `small` `default` `large` | `default | | +| `size` | one-of string | `small` `default` `large` | `default` | | | `label` | string | button description | null | | | `aria-label` | string | button description for screen readers | null | | | `aria-pressed` | boolean | `true/false` | `false` | | @@ -70,7 +70,7 @@ A few discussions were about naming and prop drilling (source here). We found th | prop | type | options | default | notes | | -- | -- | -- | -- | -- | | `variant` | one-of string | `primary` `secondary` `danger` `invisible` | `secondary` | | -| `size` | one-of string | `small` `default` `large` | `default | | +| `size` | one-of string | `small` `default` `large` | `default` | | | `label` | string | button description | null | | | `leadingVisual` | children (slot) | octicon | null | | | `trailingVisual` | children (slot) | octicon | null | | @@ -100,12 +100,13 @@ A few discussions were about naming and prop drilling (source here). We found th | prop | type | options | default | notes | | -- | -- | -- | -- | -- | -| | | | | +| `children` | child slot | | | | ## Next review session - [ ] Help refine transition animations + tokenize - [ ] Icon colors- same as button text, or specific? -- [ ] for `LinkStyledAsButton` should `trailingAction` slot be reserved for the > chevron indicating this is a link and not a button? +- [ ] for `LinkStyledAsButton` should `trailingAction` slot be reserved for the > chevron indicating this is a link and not a button? Or is it more of a `trailingVisual`? - [ ] What color should `invisible` button variant be? Blue or grey? - [ ] No aria-label for `LinkStyledAsButton` - [ ] Do we like `fullWidth` or `block` as a prop for width behavior? +- [ ] Should `ButtonGroup` offer an option with gaps (more of a layout tool) diff --git a/src/button-refactor/button-group.scss b/src/button-refactor/button-group.scss index e69de29bb2..bc27caeae8 100644 --- a/src/button-refactor/button-group.scss +++ b/src/button-refactor/button-group.scss @@ -0,0 +1,27 @@ +.ButtonGroup { + display: flex; + flex-direction: row; +} + +// reset border-radius +.ButtonGroup-item { + border-right-width: 0; + border-radius: 0; + + &:first-of-type { + border-top-left-radius: $border-radius; + border-bottom-left-radius: $border-radius; + } + + &:last-of-type { + border-right-width: $border-width; + border-top-right-radius: $border-radius; + border-bottom-right-radius: $border-radius; + } + + // ensure that the focus ring sits above the adjacent buttons + &:focus, + &:active { + z-index: 1; + } +} From b0dba371a643d6f42c7cb4d4806b05cf05524978 Mon Sep 17 00:00:00 2001 From: langermank Date: Thu, 20 Jan 2022 11:36:06 -0800 Subject: [PATCH 08/16] cleanup --- src/button-refactor/button.scss | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/button-refactor/button.scss b/src/button-refactor/button.scss index 02a865f96e..8ba4b00537 100644 --- a/src/button-refactor/button.scss +++ b/src/button-refactor/button.scss @@ -15,9 +15,6 @@ text-align: center; height: 32px; padding: 0 16px; - - // define grid slots and layout for standard button contents - // &:not(.Button--iconOnly) { display: flex; flex-direction: row; justify-content: space-between; From 479b49dc6106f81ee21457b4e13caf23213e2f29 Mon Sep 17 00:00:00 2001 From: langermank Date: Thu, 20 Jan 2022 12:46:20 -0800 Subject: [PATCH 09/16] example sheet for review --- .../explorations/ExampleSheet.stories.jsx | 691 ++++++++++++++++++ .../src/stories/explorations/Link.stories.jsx | 2 +- .../LinkStyledAsButton.stories.jsx | 2 +- src/button-refactor/button.scss | 7 + src/support/mixins/misc.scss | 16 + 5 files changed, 716 insertions(+), 2 deletions(-) create mode 100644 docs/src/stories/explorations/ExampleSheet.stories.jsx diff --git a/docs/src/stories/explorations/ExampleSheet.stories.jsx b/docs/src/stories/explorations/ExampleSheet.stories.jsx new file mode 100644 index 0000000000..0b90b15065 --- /dev/null +++ b/docs/src/stories/explorations/ExampleSheet.stories.jsx @@ -0,0 +1,691 @@ +import React from 'react' +import clsx from 'clsx' +import {ButtonTemplate} from './Button.stories' +import {IconButtonTemplate} from './IconButton.stories' +import {LinkTemplate} from './Link.stories' +import {LinkStyledAsButtonTemplate} from './LinkStyledAsButton.stories' +import {ButtonGroupTemplate} from './ButtonGroup.stories' + +export default { + title: 'Explorations', + layout: 'padded', + argTypes: {} +} + +const gridStyle = { + display: 'grid', + gridAutoFlow: 'column dense', + gap: '16px', + justifyItems: 'start' +} + +export const ExampleSheet = ({}) => ( +
+

Standard

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Leading visual

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Trailing visual

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Leading + Trailing visual

+
+ + + + +
+
+ + + + +
+
+ + + + +
+ +

Trailing action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+ +

Leading visual + action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Trailing visual + action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Leading + Trailing visual + action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+

IconButton

+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+

LinkStyledAsButton

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Leading visual

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Trailing visual

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Leading + Trailing visual

+
+ + + + +
+
+ + + + +
+
+ + + + +
+ +

Trailing action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+ +

Leading visual + action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Trailing visual + action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Leading + Trailing visual + action

+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+

Link

+
+ + +
+
+ + +
+
+) + +// export const Playground = ExampleSheetTemplate.bind({}) +// Playground.args = {} diff --git a/docs/src/stories/explorations/Link.stories.jsx b/docs/src/stories/explorations/Link.stories.jsx index 81a0194d67..7379937b1f 100644 --- a/docs/src/stories/explorations/Link.stories.jsx +++ b/docs/src/stories/explorations/Link.stories.jsx @@ -89,7 +89,7 @@ const arrow = ( ) diff --git a/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx b/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx index 3387d1244a..70bd091e83 100644 --- a/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx +++ b/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx @@ -171,7 +171,7 @@ const arrow = ( ) diff --git a/src/button-refactor/button.scss b/src/button-refactor/button.scss index 8ba4b00537..55a0539ed7 100644 --- a/src/button-refactor/button.scss +++ b/src/button-refactor/button.scss @@ -21,6 +21,13 @@ align-items: center; gap: $spacer-2; + // mobile friendly sizing + @media (pointer: course) { + &::before { + @include minTouchTarget(48px, 48px); + } + } + // base states &:hover, diff --git a/src/support/mixins/misc.scss b/src/support/mixins/misc.scss index 05d908a1d9..101a012cc1 100644 --- a/src/support/mixins/misc.scss +++ b/src/support/mixins/misc.scss @@ -31,3 +31,19 @@ outline: 2px solid var(--color-accent-fg); outline-offset: 2px; } + +// if min-width is undefined, return only min-height +@mixin minTouchTarget($min-height: 48px, $min-width: '') { + position: absolute; + top: 50%; + left: 50%; + width: 100%; + height: 100%; + min-height: $min-height; + content: ''; + transform: translateX(-50%) translateY(-50%); + + @if $min-width != '' { + min-width: $min-width; + } +} From 19436675b120c655e9f9857525a1f0cf96b2c5ad Mon Sep 17 00:00:00 2001 From: langermank Date: Thu, 20 Jan 2022 12:59:51 -0800 Subject: [PATCH 10/16] most giant story of all time --- .../explorations/ExampleSheet.stories.jsx | 295 ++++++++++++++++++ 1 file changed, 295 insertions(+) diff --git a/docs/src/stories/explorations/ExampleSheet.stories.jsx b/docs/src/stories/explorations/ExampleSheet.stories.jsx index 0b90b15065..2f539aa70f 100644 --- a/docs/src/stories/explorations/ExampleSheet.stories.jsx +++ b/docs/src/stories/explorations/ExampleSheet.stories.jsx @@ -19,6 +19,14 @@ const gridStyle = { justifyItems: 'start' } +const gridStyleStretch = { + display: 'grid', + gridAutoFlow: 'column', + gap: '16px', + justifyItems: 'start', + width: '100%' +} + export const ExampleSheet = ({}) => (

Standard

@@ -118,6 +126,293 @@ export const ExampleSheet = ({}) => (
+

Fullwidth (all visual scenarios, one button size)

+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+ +
+

Fullwidth (all visual scenarios, one button size) visualPosition fixed

+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+
+ + + + +
+

Leading visual + action

From 871167c94e04ce985cf9d976b0da694d7ec4bef2 Mon Sep 17 00:00:00 2001 From: langermank Date: Tue, 25 Jan 2022 07:42:18 -0800 Subject: [PATCH 11/16] lg button height, segment control discussion --- docs/src/stories/explorations/Button.stories.jsx | 2 +- docs/src/stories/explorations/IconButton.stories.jsx | 2 +- docs/src/stories/explorations/LinkStyledAsButton.stories.jsx | 2 +- src/button-refactor/ButtonsLinksAPI.md | 3 +++ src/button-refactor/button.scss | 2 +- 5 files changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/src/stories/explorations/Button.stories.jsx b/docs/src/stories/explorations/Button.stories.jsx index 6e99b67dbb..c755318ba0 100644 --- a/docs/src/stories/explorations/Button.stories.jsx +++ b/docs/src/stories/explorations/Button.stories.jsx @@ -31,7 +31,7 @@ export default { mapping: [null, 'Button--small', 'Button--large'], // values control: { type: 'inline-radio', - labels: ['default [32px]', 'small [28px]', 'large [44px]'] + labels: ['default [32px]', 'small [28px]', 'large [40px]'] }, table: { category: 'CSS' diff --git a/docs/src/stories/explorations/IconButton.stories.jsx b/docs/src/stories/explorations/IconButton.stories.jsx index f03d53481c..83acd9c942 100644 --- a/docs/src/stories/explorations/IconButton.stories.jsx +++ b/docs/src/stories/explorations/IconButton.stories.jsx @@ -31,7 +31,7 @@ export default { mapping: [null, 'Button--small', 'Button--large'], // values control: { type: 'inline-radio', - labels: ['default [32px]', 'small [28px]', 'large [44px]'] + labels: ['default [32px]', 'small [28px]', 'large [40px]'] }, table: { category: 'CSS' diff --git a/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx b/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx index 70bd091e83..32bef167d4 100644 --- a/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx +++ b/docs/src/stories/explorations/LinkStyledAsButton.stories.jsx @@ -36,7 +36,7 @@ export default { mapping: [null, 'LinkStyledAsButton--small', 'LinkStyledAsButton--large'], // values control: { type: 'inline-radio', - labels: ['default [32px]', 'small [28px]', 'large [44px]'] + labels: ['default [32px]', 'small [28px]', 'large [40px]'] }, table: { category: 'CSS' diff --git a/src/button-refactor/ButtonsLinksAPI.md b/src/button-refactor/ButtonsLinksAPI.md index fbe1dc04e6..2d1f051642 100644 --- a/src/button-refactor/ButtonsLinksAPI.md +++ b/src/button-refactor/ButtonsLinksAPI.md @@ -28,6 +28,8 @@ Existing issues -- Allie's thoughts on limiting icon only buttons and working towards encouraging visual labels - https://github.com/github/primer/discussions/477 -- Buttons styled as links, links styled as buttons +- https://github.com/github/primer/issues/65 + -- Segmented control, pressed state discussion ## Specific notes from previous bugs to keep in mind - [ ] Check all button variants have shadow present @@ -110,3 +112,4 @@ A few discussions were about naming and prop drilling (source here). We found th - [ ] No aria-label for `LinkStyledAsButton` - [ ] Do we like `fullWidth` or `block` as a prop for width behavior? - [ ] Should `ButtonGroup` offer an option with gaps (more of a layout tool) +- [ ] Should `Button` proper handle a `pressed` (formerly selected) state, or should we create a new component `SegmentedControl` that uses `Button` and provides specific design (color, shadows)? diff --git a/src/button-refactor/button.scss b/src/button-refactor/button.scss index 55a0539ed7..eadde5350f 100644 --- a/src/button-refactor/button.scss +++ b/src/button-refactor/button.scss @@ -102,7 +102,7 @@ } .Button--large { - height: 44px; + height: 40px; } .Button--fullWidth { From bf8efe2d45c3813d4373a339b1fa19d6e90ac332 Mon Sep 17 00:00:00 2001 From: langermank Date: Tue, 22 Mar 2022 15:12:14 -0700 Subject: [PATCH 12/16] play with tokens --- .../stories/explorations/Button.stories.jsx | 21 ++-- .../LinkStyledAsButton.stories.jsx | 15 ++- docs/src/stories/explorations/adr.md | 118 ++++++++++++++++++ src/button-refactor/button.scss | 101 +++++++++++++-- .../link-styled-as-button.scss | 4 +- 5 files changed, 227 insertions(+), 32 deletions(-) create mode 100644 docs/src/stories/explorations/adr.md diff --git a/docs/src/stories/explorations/Button.stories.jsx b/docs/src/stories/explorations/Button.stories.jsx index c755318ba0..87e92c429c 100644 --- a/docs/src/stories/explorations/Button.stories.jsx +++ b/docs/src/stories/explorations/Button.stories.jsx @@ -4,10 +4,6 @@ import clsx from 'clsx' export default { title: 'Explorations/Button', parameters: { - // design: { - // type: 'figma', - // url: 'https://www.figma.com/file/GCvY3Qv8czRgZgvl1dG6lp/Primer-Web?node-id=4371%3A7128' - // }, layout: 'padded' }, @@ -39,19 +35,19 @@ export default { description: 'Controls button height', defaultValue: 0 }, - visualPosition: { + alignContent: { options: [0, 1], // iterator - mapping: [null, 'Button-content--visualFixed'], // values + mapping: [null, 'Button-content--alignStart'], // values control: { type: 'inline-radio', - labels: ['default', 'fixed'] + labels: ['center [default]', 'start'] }, table: { category: 'CSS' }, description: - '[Name TBD!] Controls where the leading or trailing visuals position themselves in a fullWidth button (lock to text label or button bounds)', - defaultValue: 'Button-content--visualFixed' + 'Align button label + visuals to the center (default for CTA buttons) or start for select/dropdown button scenarios', + defaultValue: 0 }, label: { defaultValue: 'Button', @@ -171,9 +167,9 @@ export const ButtonTemplate = ({ pressed, focusElement, active, - visualPosition, className, - ariaLabel + ariaLabel, + alignContent }) => ( <>