Add support for trailing and leading visual icons in Primer::Beta::Link (second try)

[HDinger]

What are you trying to accomplish?

This is a follow-up of #3040. It re-adds the with_leading_icon and with_trailing_icon slots to Links. The original PR was reverted, because:

The underlined text (initially and on hover) extends past the beginning and end of the text.

This is a result of the additional spacing that is added in between inline elements by the browser.

Screenshots

Before

After

List the issues that this change affects.

Closes #2981

What approach did you choose and why?

One possible solution is to render the icons outside the tag itself and set their foreground color to the link color. In my eyes, this approach has two main disadvantages:

1. The icons would look like they are part of the link, but they are not clickable.
2. We would have to duplicate/inherit all different possible link styles ("primary", "secondary", "default", "muted") for the icons

I tried out a different approach, which is also not ideal, but still better in my eyes: I added a span around the whole content (including the icons) resulting in the following HTML:

<a  class="Link">
  <span class="Link-content">
     <svg class="octicon octicon-accessibility"><path  ...></path></svg>
     This is a link!
      <svg class="octicon octicon-bell"><path ...></path></svg>
  </span>
</a>

Thus, I can set display: inline-flex on the Link-content span which omits all additional white spaces that inline elements normally have while leaving the acutal <a> tag unchanged.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Lookbook)
• 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: cc5c5f3

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

This PR includes changesets to release 1 package

Name	Type
@primer/view-components	Minor

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

[HDinger]

Hi @camertron I addressed your feedback from #3040. I chose however a different approach which I tried to explain above. I would be intereseted in your opinion on this.

[HDinger]

Hi @camertron any news on this?

[HDinger]

Hey folks :) is it possible to get any feedback on this? /cc @camertron

[camertron]

Hey @HDinger my sincere apologies for letting this lapse for so long 😬 I'll try to follow up more quickly next time!

I copied this PR into the primer org so our visual regression tests (VRT) would run, and I'm seeing a number of issues in this PR that I'm hoping can be addressed before merge. Could you take a look at these? https://github.com/primer/view_components/pull/3150/files

You should be able to run the VRT tests and generate snapshots locally by running npx playwright install --with-deps followed by ./script/run-playwright. The generated screenshots in the repo were generated on a Linux machine and look slightly different if generated on a Mac (so annoying, I know 🙄) so you might have to spin up an Ubuntu container to effectively compare before/after screenshots. Happy to help figure that out, please let me know 👍

[HDinger]

Thanks for detailed explanation 🙇 I will have a look in the next days 👍

[HDinger]

Hi @camertron there was indeed a change in the line-height. Locally the images now look fine. Could you please check here?

[camertron]

@HDinger Ok, the snapshots are looking much better, thank you 😄 Could you rebase this PR on #3150 and address the remaining build failures? Thanks!

[HDinger]

@HDinger Ok, the snapshots are looking much better, thank you 😄 Could you rebase this PR on #3150 and address the remaining build failures? Thanks!

@camertron Done 👍 The semver label run is still red, but I honestly don't know what to do about that

[camertron]

Yeah, the semver check is running into a permissions issue, we can ignore it.