Skip to content

fix: Remove underline from trailing whitespace in external links #3709

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xyos
Copy link

@xyos xyos commented Jul 31, 2025

Description

Removes the non-breaking space that caused underlines to appear below trailing whitespace between external icon and link text. Replaces it with margin-inline-start.

Related links, issue #, if available: AWSUI-60840

How has this been tested?

I tested the link permutation page at http://localhost:8081/#/light/link/permutations manually and generated a diff to check difference between changes and previous implementation using odiff:

diff3
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@xyos xyos force-pushed the fix-link-underline branch from 4425ba7 to 2b63d72 Compare August 4, 2025 09:48
Removes the non-breaking space that caused underlines to appear below
trailing whitespace between external icon and link text. Replaces it
with margin-inline-start.

AWSUI-60840
@xyos xyos force-pushed the fix-link-underline branch from 2b63d72 to a4c1cc5 Compare August 4, 2025 13:46
@xyos xyos marked this pull request as ready for review August 4, 2025 13:49
@xyos xyos requested a review from a team as a code owner August 4, 2025 13:49
@xyos xyos requested review from avinashbot and removed request for a team August 4, 2025 13:49
Copy link

codecov bot commented Aug 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.99%. Comparing base (da6e199) to head (ad348b7).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #3709    +/-   ##
========================================
  Coverage   96.99%   96.99%            
========================================
  Files         820      820            
  Lines       23799    23799            
  Branches     7917     8338   +421     
========================================
  Hits        23084    23084            
  Misses        709      709            
  Partials        6        6            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@avinashbot
Copy link
Member

@xyos Hi! Thanks for opening the pull request, taking a closer look and running a full screenshot comparison test.

Though there is one regression that should be addressed. Being the one who made the original change with the  , the issue that I was fixing was that the external icon would sometimes wrap into the next line on its own, and it wasn't ideal to have an icon just hanging out on a new line like that. You can see the issue on the #/light/link/icon-overflow-permutations dev page (current state on the left, this PR on the right).

Screenshot 2025-08-05 at 16 40 50

I recall trying to do the \a0 pseudoelement trick, but it didn't quite work for me. But perhaps there's a way of making it work with a little bit more experimentation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants