-
Notifications
You must be signed in to change notification settings - Fork 1.3k
WIP: Button/Link API exploration #1874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
9ebd6cd
f15a0ac
14e9185
1c67f02
d148ec0
6cc2f68
3d88221
b0dba37
479b49d
1943667
871167c
bf8efe2
488e665
155ae23
a021941
f67a1c2
d2308e2
2b1c7c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 = ( | ||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" width="16" height="16"> | ||
| <path | ||
| fill-rule="evenodd" | ||
| d="M8 0C3.58 0 0 3.58 0 8c0 3.54 2.29 6.53 5.47 7.59.4.07.55-.17.55-.38 0-.19-.01-.82-.01-1.49-2.01.37-2.53-.49-2.69-.94-.09-.23-.48-.94-.82-1.13-.28-.15-.68-.52-.01-.53.63-.01 1.08.58 1.23.82.72 1.21 1.87.87 2.33.66.07-.52.28-.87.51-1.07-1.78-.2-3.64-.89-3.64-3.95 0-.87.31-1.59.82-2.15-.08-.2-.36-1.02.08-2.12 0 0 .67-.21 2.2.82.64-.18 1.32-.27 2-.27.68 0 1.36.09 2 .27 1.53-1.04 2.2-.82 2.2-.82.44 1.1.16 1.92.08 2.12.51.56.82 1.27.82 2.15 0 3.07-1.87 3.75-3.65 3.95.29.25.54.73.54 1.48 0 1.07-.01 1.93-.01 2.2 0 .21.15.46.55.38A8.013 8.013 0 0016 8c0-4.42-3.58-8-8-8z" | ||
| ></path> | ||
| </svg> | ||
| ) | ||
|
|
||
| const caret = ( | ||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" width="16" height="16"> | ||
| <path d="M4.427 7.427l3.396 3.396a.25.25 0 00.354 0l3.396-3.396A.25.25 0 0011.396 7H4.604a.25.25 0 00-.177.427z"></path> | ||
| </svg> | ||
| ) | ||
|
|
||
| export const ButtonTemplate = ({ | ||
| label, | ||
| variant, | ||
| disabled, | ||
| size, | ||
| fullWidth, | ||
| leadingVisual, | ||
| trailingVisual, | ||
| trailingAction, | ||
| pressed, | ||
| focusElement, | ||
| focusAllElements | ||
| }) => ( | ||
| <> | ||
| <button | ||
| disabled={disabled} | ||
| className={clsx( | ||
| 'Button', | ||
| variant && `${variant}`, | ||
| size && `${size}`, | ||
| fullWidth && 'btn-block', | ||
| focusAllElements && 'focus' | ||
| )} | ||
| aria-pressed={pressed} | ||
| > | ||
| {/* {leadingVisual && <span className="" dangerouslySetInnerHTML={{__html: leadingVisual}} />} */} | ||
| {leadingVisual && <span className="Button--visual Button--leadingVisual">{mona}</span>} | ||
| <span className="Button--label">{label}</span> | ||
| {trailingVisual && <span className="Button--visual Button--trailingVisual">{mona}</span>} | ||
| {trailingAction && <span className="Button--visual Button--trailingAction">{caret}</span>} | ||
| </button> | ||
| {focusElement && focusMethod()} | ||
| </> | ||
| ) | ||
|
|
||
| export const Playground = ButtonTemplate.bind({}) | ||
| Playground.args = { | ||
| focusElement: false, | ||
| focusAllElements: false, | ||
| leadingVisual: false, | ||
| trailingAction: false, | ||
| trailingVisual: false | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% on what this means, is it about (to use a flex analogy)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and I'm struggling to name this prop. I plan to ask the pattern working group how much granularity we need for positioning visuals in a button. The way I see it, we either allow each visual to have a prop (align to the button bounds, or align to the text) OR we are prescriptive and just have one prop where all visuals act the same way. The way I handled it looks similar to Sid but I'm not sure we have any left-aligned text in buttons 🤔
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if we already have any left-aligned buttons but here's an example that Sid was ideating on - imo if we don't already need that flexibility we should avoid introducing it and be prescriptive as you say, although I do think having actions locked to the container and visuals locked to the text looks more intuitive.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need an option for alignment with text? I'd imagine that you'd sometimes want
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh see that's where I also get 🥴 I feel like Which makes me think we still need a subtle Button variant. When you want a Button that's.. subtle 🙃
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess there are two use cases for buttons styled as links:
And some ideas for how to accommodate both of those cases:
FWIW I think they can be in line with text but have padding and negative margin so they still have accessible tap targets, but maybe that's a different conversation. Also curious how Far too many thoughts for one comment, I'm sorry 🙃 |
||
|
|
||
| ## Component design specs | ||



There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree this seems like a requirement. See primer/react#1733 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coming from Primer Patterns Working group recording, +1 for not having
ButtonStyledAsLinkat all and using Invisible Button variant for that.see also: @ashygee's proposal of the feature I mentioned in the above PR: https://github.com/github/primer/discussions/477#discussioncomment-1915419