Remove link button

[pksjce]

Describe your changes here.

With some feedback from @colebemis and my personal experience trying to use it in memex, I see no use for LinkButton. The normal Button component can have the as polymorphic prop. So I removed it and fixed up all the associated tests and stories.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: ae5a424

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	64.24 KB (0%)
dist/browser.umd.js	64.6 KB (0%)

[siddharthkp]

Can you reply on https://github.com/github/primer/discussions/477 as well, curious to here if this matches other implementations as well

[mperrotti]

We've discussed this in the Primer patterns working session, and it aligns with what they're doing in PCSS and PVC.

cc @langermank

[langermank]

We've discussed this in the Primer patterns working session, and it aligns with what they're doing in PCSS and PVC.

cc @langermank

I'm not sure it does align. These are the components we discussed creating in the realm of Buttony things (see notes)

Edit: naming is confusing, I think this DOES align in that it seems to be removing a button styled as a link, not a link styled as a button.. right?! 😆 If that's the case then 👍 I'll leave those notes below as a reference point regardless

Component	Description
Button	standard button with variants, size, visual slots
IconButton	button with icon only (square) and required aria-label
ReactionButton	button snowflake with specific styles/interaction design (round) potentially a variant
ButtonGroup	wrapper to handle grouping buttons
Link	a with variants, optional trailing visuals
LinkStyledAsButton	a with button variants, required trailing visual

We wanted to have LinkStyledAsButton or LinkButton to enforce a trailing icon if its a link styled as a button (to indicate navigational behavior) and also separate concerns from a semantic button and a href. There are props that are allowed for a button that a a styled as a button wouldn't need, such as aria-label and aria-pressed

[colebemis]

it seems to be removing a button styled as a link, not a link styled as a button.. right?!

I don't think so. LinkButton is a link styled as a button.

We wanted to have LinkStyledAsButton or LinkButton to enforce a trailing icon if its a link styled as a button (to indicate navigational behavior) and also separate concerns from a semantic button and a href.

My initial understanding was that LinkButton (aka LinkStyledAsButton) and Button were basically the same components. But if there's a clear functional difference between LinkButton and Button, then I think it makes sense to keep it around.

[langermank]

My initial understanding was that LinkButton (aka LinkStyledAsButton) and Button were basically the same components. But if there's a clear functional difference between LinkButton and Button, then I think it makes sense to keep it around.

I think this would be a good time to start drafting an ADR based on the discussions in the pattern working group! I can start it, I've just been focusing on my main Q3 priorities the past 2 weeks.

Until then, this is a question of allowing components to use as to change their semantic meaning. Something we discussed in the working group:

Separate components based on logic? One component that handles multiple kinds of logic?
- Broken up by functional logic not design logic (e.g.: `<IconButton>`, not `<Button iconOnly=true leadingVisual=MyIcon>`)
- When breaking up components, consider how the markup would need to change to fit the intended functionality.

These are the notes from Jan 18

We felt that since this is semantically a href it should be its own component, inheriting styles and props from Button proper. Since its a link, it has its own set of props like target and excludes the aria labels I mentioned earlier. Additionally, we'll either enforce a specific octicon to show as a trailingVisual or provide it as a prop. Here's my prototype of that logic.

Its been a few weeks since I've been able to focus on these patterns, and I hate to be a blocker here. But I think it would be great to get Button2 in line with what we've been drafting/discussing as the "future" of how we handle Buttony things.

[pksjce]

I decided to remove it because implementation-wise it does not add anything to the underlying Button component. The underlying component has to already support the polymorphic as property for all of this to work. So technically product teams can ignore LinkButton and use the Button if they want to. It will work already

Another point, unlike the IconButton (which is a very common use case), LinkButton use cases are so few. I realised this as I tried to replace Button components in memex. Hence, I felt it does not justify the overhead of having a new component and associated documentation. It would be practical to remove it.

What I'm hearing in this discussion is the opposite and that LinkButton stays?
In simpler terms is it a yay or nay on the LinkButton?