WIP: Button/Link API exploration #1874

langermank · 2022-01-13T17:16:12Z

Design and API exploration for buttony and linky components: Button, IconButton, Link, LinkStyledAsButton, ButtonGroup, ReactionButton

Working notes and prop tables can be found in this readme

Figma draft

Progress tracking

Gather insights from existing issues and discussions
Create base component templates for stress testing/markup + draft CSS
Discuss initial draft with Pattern Working Group
Edit naming conventions and design based on Pattern Working Group feedback
Meet with group again to discuss nitty-gritty things (mainly naming and style)
Request detailed markup and CSS review
Begin documenting breaking changes between old Button and this new refactor

changeset-bot · 2022-01-13T17:16:14Z

⚠️ No Changeset found

Latest commit: 2b1c7c8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

pksjce · 2022-01-14T01:43:59Z

src/button-refactor/ButtonsLinksAPI.md

+| -- | -- |
+| 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 |


Agree this seems like a requirement. See primer/react#1733 (comment)

coming from Primer Patterns Working group recording, +1 for not having ButtonStyledAsLink at 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

pksjce · 2022-01-14T01:44:44Z

src/button-refactor/ButtonsLinksAPI.md

+
+| prop | type | options | default | notes |
+| -- | -- | -- | -- | -- |
+| `variant` | one-of string | `primary` `secondary` `danger` `outline`? | `secondary` | |


invisible is missing here?

If I'm interpreting this correctly, invisible would become ButtonStyledAsLink and maintain the hover background? I wonder what that means for IconButton which is often styled as invisible (PVC's IconButton is invisible) IconButtonStyledAsLink is a mouthful 😬

Yes, @pouretrebelle's interpretation is correct! Thanks for pointing out that use case.. I'll make note of this for the pattern working group to discuss. I think Ash had an idea for a solution to this based on the Button audit.

pksjce · 2022-01-14T04:00:56Z

To gain full context, is this a refactor of existing design or is there scope to make any design changes?

A few I can think that came up during the react implementation

Block buttons - where should the icon be positioned? (Button v2: Fix icon positions for block buttons react#1733)
Buttons that look like links (seems like this is being fixed)
Should Outline Button exist? (https://github.com/github/primer/discussions/278)
Do we need large button? (https://github.com/github/primer/issues/253)
Focus outline discussion (https://github.com/github/primer/discussions/414)

How much of this do you think can get addressed in this iteration?

pouretrebelle · 2022-01-14T12:06:36Z

src/button-refactor/ButtonsLinksAPI.md

+| `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 |


I'm not 100% on what this means, is it about (to use a flex analogy) center vs space-between for full-width buttons that have leading/trailing visuals? I like the solution Sid posted here for spacing visuals and actions with text, do we need more design flexibility than that?

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 🤔

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.

pouretrebelle · 2022-01-14T12:22:29Z

src/button-refactor/ButtonsLinksAPI.md

+| `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 |


Do we need an option for alignment with text? I'd imagine that you'd sometimes want ButtonStyledAsLink to be inline and other times want it to line up with other buttons 🥴

Ahh see that's where I also get 🥴

I feel like ButtonStyledAsLink means you want to hide the fact that this is actually a button.. so its inline, blue text, not centered text.. but retains height for accessibility?

Which makes me think we still need a subtle Button variant. When you want a Button that's.. subtle 🙃

I guess there are two use cases for buttons styled as links:

Secondary CTAs in forms or modals, e.g.

Downplaying visual weight, where they should always be accompanied by an icon/action, and are in line with text e.g.

And some ideas for how to accommodate both of those cases:

The first one is some subtle/invisible Button variant and the second is a ButtonStyledAsLink (so BSAL always requires a trailing icon/action)
It feels a bit weird that you could achieve a similar visual style for button elements using different combinations of components/props, I feel like that's how we got into this mess?

They're both ButtonStyledAsLink but we have a display prop to toggle between block/inline
Then it's a bit ambiguous when icon/action are required, only when they're not locked up with other buttons?

They're both ButtonStyledAsLink and are inline, but we use a button group component for vertical alignment of locked-up buttons

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 <summary> comes in to this discussion, if at all 🥴

Far too many thoughts for one comment, I'm sorry 🙃

pouretrebelle · 2022-01-14T12:24:31Z

src/button-refactor/ButtonsLinksAPI.md

+| 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 |


When would we not want this to be a chevron (or potentially link-external)? I think that should be a feature of LinkStyledAsButton rather than a required option.

langermank · 2022-01-18T04:37:59Z

To gain full context, is this a refactor of existing design or is there scope to make any design changes?

There's scope to make design changes, but mostly around consistency in spacing, sizing and alignment. Potentially changing some variants.

How much of this do you think can get addressed in this iteration?

We are covering everything in that list 😸 The outline styles is actually separate, but should be released soon #1744

mperrotti · 2022-01-18T18:51:24Z

Just a heads up - the latest Storybook deployment throws an error for the Buttons explorations:

ariaLabel is not defined

langermank · 2022-03-03T23:40:28Z

src/button-refactor/button.scss

+// button child elements
+
+// align svg
+.Button-visual {


pointer-events: none;

Co-authored-by: Vinicius Depizzol <vdepizzol@gmail.com>

github-actions · 2022-05-22T03:22:03Z

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

langermank · 2022-10-17T21:20:51Z

Closing in favor of the PVC implementation ✨

begin button api refactor

9ebd6cd

langermank added the DO NOT MERGE label Jan 13, 2022

setup button playground

f15a0ac

github-actions bot deployed to Preview January 14, 2022 01:33 View deployment

github-actions bot deployed to Storybook Preview January 14, 2022 01:33 View deployment

pksjce reviewed Jan 14, 2022

View reviewed changes

pouretrebelle reviewed Jan 14, 2022

View reviewed changes

more config

14e9185

github-actions bot deployed to Preview January 18, 2022 04:18 View deployment

github-actions bot deployed to Storybook Preview January 18, 2022 04:18 View deployment

langermank added 2 commits January 18, 2022 13:10

stash meeting notes

1c67f02

WIP

d148ec0

github-actions bot deployed to Preview January 20, 2022 00:29 View deployment

github-actions bot deployed to Storybook Preview January 20, 2022 00:29 View deployment

slight refactor to handle trailingAction locked

6cc2f68

github-actions bot deployed to Preview January 20, 2022 18:41 View deployment

github-actions bot deployed to Storybook Preview January 20, 2022 18:41 View deployment

langermank added 3 commits January 20, 2022 11:32

button group

3d88221

cleanup

b0dba37

example sheet for review

479b49d

github-actions bot deployed to Preview January 20, 2022 20:46 View deployment

github-actions bot deployed to Storybook Preview January 20, 2022 20:46 View deployment

most giant story of all time

1943667

github-actions bot deployed to Preview January 25, 2022 15:43 View deployment

github-actions bot deployed to Storybook Preview January 25, 2022 15:43 View deployment

mperrotti mentioned this pull request Jan 25, 2022

Button v2: Fix icon positions for block buttons primer/react#1733

Closed

5 tasks

pksjce mentioned this pull request Feb 2, 2022

Add selected state to Button primer/react#1819

Closed

6 tasks

langermank commented Mar 3, 2022

View reviewed changes

src/button-refactor/button.scss

// button child elements

// align svg

.Button-visual {

Copy link

Contributor Author

langermank Mar 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointer-events: none;

vdepizzol mentioned this pull request Mar 3, 2022

Refactor Flash alert #1979

Closed

1 task

play with tokens

bf8efe2

github-actions bot deployed to Preview March 22, 2022 22:12 View deployment

github-actions bot deployed to Storybook Preview March 22, 2022 22:12 View deployment

design refinements

488e665

Co-authored-by: Vinicius Depizzol <vdepizzol@gmail.com>

github-actions bot deployed to Preview March 23, 2022 03:09 View deployment

github-actions bot deployed to Storybook Preview March 23, 2022 03:09 View deployment

github-actions bot added the Stale Automatically marked as stale. label May 22, 2022

langermank removed the Stale Automatically marked as stale. label May 26, 2022

langermank added 2 commits July 14, 2022 01:52

Merge branch 'main' of https://github.com/primer/css into button-api

155ae23

support tag variation for pvc

a021941

langermank temporarily deployed to github-pages July 14, 2022 02:09 Inactive

github-actions bot temporarily deployed to Storybook Preview July 14, 2022 02:10 Inactive

tokens

f67a1c2

langermank temporarily deployed to github-pages July 14, 2022 02:38 Inactive

github-actions bot temporarily deployed to Storybook Preview July 14, 2022 02:38 Inactive

update invisible style

d2308e2

langermank temporarily deployed to github-pages August 19, 2022 22:44 Inactive

github-actions bot temporarily deployed to Storybook Preview August 19, 2022 22:45 Inactive

Merge branch 'main' into button-api

2b1c7c8

jonrohan temporarily deployed to github-pages August 23, 2022 20:25 Inactive

github-actions bot temporarily deployed to Storybook Preview August 23, 2022 20:26 Inactive

langermank closed this Oct 17, 2022

WIP: Button/Link API exploration #1874

WIP: Button/Link API exploration #1874

Uh oh!

Conversation

langermank commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress tracking

Uh oh!

changeset-bot bot commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pksjce commented Jan 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pouretrebelle Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

langermank Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

langermank commented Jan 18, 2022

Uh oh!

mperrotti commented Jan 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 22, 2022

Uh oh!

langermank commented Oct 17, 2022

Uh oh!

Uh oh!

langermank commented Jan 13, 2022 •

edited

Loading

changeset-bot bot commented Jan 13, 2022 •

edited

Loading

pouretrebelle Jan 18, 2022 •

edited

Loading

langermank Jan 18, 2022 •

edited

Loading