USWDS - Links: Fix external link icon issues by mejiaj · Pull Request #5046 · uswds/uswds

mejiaj · 2022-11-18T22:09:12Z

Description

uswds-1918-2022-11-21.at.14.50.09.webm

Fixed icon wrapping and safari display issues in external links. Improved wrapping so icon doesn't break on its own and causes display issues with punctuation. There's also an improved way for getting around Safari display issues (transparent gradient). Closes #4977.

add-color-icon mixin now allows more control over positioning. The $icon-object that gets passed to add-color-icon mixin now takes attributes for x and y positioning. These are used for both background and mask positioning.

Defaults

position-y: center
position-x: center

Additional information

This has been a tricky issue to debug and fix. The original attempt (#3395) later caused a background bleed issue in Safari (#4439). Now both issues should be resolved.

Link and icon wrapping bug

Both the link and pseudoelement have been made inline elements. The icon sizing is controlled via padding-left and background/mask size.

Safari display bug

Icon with transparent gradient on mask

Background bleeding has been resolved by adding a transparent linear gradient to masks.

Related issues

Browsers tested

Safari 16
Edge 107.0.1418.42 (chromium)
Firefox 108

How to test

I've also created a test branch in Site you can use for additional testing.

There shouldn't be any visual regressions in external links
There shouldn't be any visual regressions with external links in Safari
There shouldn't be any visual regressions in external link background image fallback
Punctuation shouldn't break into it's own line following external link
External link icon shouldn't break into it's own line

Before you hit Submit, make sure you’ve done whichever of these applies to you:

Follow the 18F Front End Coding Style Guide and Accessibility Guide.
Run npm test and make sure the tests for the files you have changed have passed.
Run your code through HTML_CodeSniffer and make sure it’s error free.
Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

- Add SassDoc comments to mixin - Allow user to set positioning in icon object

- External links are now inline to prevent wrapping issues #3395 #4977 - $icon-object now takes x and y position settings for add-color-icon mixin - add-color-icon sets transparent gradient for Safari display issue #4434

- Remove non-breaking space in favor of margin-left - Use padding-left for creating icon width - Improve alignment via vertical align middle

mejiaj · 2022-11-21T21:16:16Z

 // [base]-[variant].svg based on the specified background-color
-
+//
+// @param {Map} - $icon-object - name, svg-height, svg-width, height, container-height, container-width, color, color-variant, color-hover, rotate, path, background-p


Added Sassdoc style comments to document the types of things $icon-object accepts in this mixin.

mejiaj · 2022-11-21T21:16:57Z

+  $position-y: if(
+    map.has-key($icon-object, "position-y"),
+    map.get($icon-object, "position-y"),
+    bottom
+  );
+  $position-x: if(
+    map.has-key($icon-object, "position-x"),
+    map.get($icon-object, "position-x"),
+    center
+  );


New settings to allow user more control over icon positioning for background/mask image.

Defaults for both are center.

mejiaj · 2022-11-21T21:17:22Z

+    mask-position: $position-x $position-y;
+    mask-size: $width $height;
+    mask-repeat: no-repeat;
+


Stopped using shorthands for background and mask properties to allow user more control in overrides.

jhancock532 · 2022-11-23T12:14:02Z

I'm not certain, but I believe display: inline-block; was added due to an issue with how the external link icon renders in Firefox. After removing the display: inline-block; rule, resizing the container causes the icon render incorrectly at pixel specific widths.

A potential solution is to use position: relative; for the link icon instead of position: absolute;, changing margin positioning to bottom: 2ex; or similar.

amyleadem

@mejiaj
Nice fix! I like the ability to customize positioning.

Everything seems to be behaving in Firefox, Chrome, and Edge. I did notice that Safari is still showing icon line breaks in the footer and identifier. However, I do not see the same issue in any of the external links in the main content of the links page in Safari.

I noticed that if I delete this extra space in the html via inspector, it resolves the issue. So I suspect this is more of an issue with uswds-site markup, but wanted to flag it for your consideration in case something else is at play.

Safari, 1040px

iOS Safari

There shouldn't be any visual regressions in external links
There shouldn't be any visual regressions with external links in Safari
There shouldn't be any visual regressions in external link background image fallback
Punctuation shouldn't break into it's own line following external link
External link icon shouldn't break into it's own line

mejiaj · 2022-11-30T22:00:41Z

Hey @jhancock532, thanks for taking a look at this PR. Were you experiencing the same issues in the test preview for the site?

If so, would you mind sharing what OS and browser (with version) you used?

I'll also take a look at that spacing issue you mentioned @amyleadem, thanks!

jhancock532 · 2022-12-01T14:37:10Z

@mejiaj I was testing this on MacOS Monterey with the latest version of Firefox at that time (107), but I'm unable to replicate this issue using Firefox and the testing site. I think the code changes you've made here have resolved the problem.

mejiaj · 2022-12-01T14:48:58Z

Fantastic, thanks for checking @jhancock532.

mejiaj · 2022-12-01T22:22:47Z

@amyleadem after looking into it some more these whitespaces are specific to site. Created a separate issue for identifier links uswds/uswds-site#1928. I'll push a fix and re-request review. Good find!

mejiaj · 2022-12-02T14:50:12Z

This PR is ready for review. The safari whitespace issue in identifier links has been fixed in uswds/uswds-site@dbb6795.

This PR must be merged before the site PR.

amyleadem

Looks good!

thisisdano

This is an important fix. Nice work!

James Mejia added 6 commits November 18, 2022 12:24

Add positioning to add-color-icon mixin

830e029

- Add SassDoc comments to mixin - Allow user to set positioning in icon object

Fix display issues with external links

37f79a8

- External links are now inline to prevent wrapping issues #3395 #4977 - $icon-object now takes x and y position settings for add-color-icon mixin - add-color-icon sets transparent gradient for Safari display issue #4434

Restore absolute icon positioning, but use inline for parent

763f39f

Make external link inline

fb0ca0e

- Remove non-breaking space in favor of margin-left - Use padding-left for creating icon width - Improve alignment via vertical align middle

Add positioning to mixin parameter docs

fcc8f8c

Update default y positioning

1574df4

mejiaj mentioned this pull request Nov 21, 2022

Identifier & footer external link wrapping in safari uswds/uswds-site#1918

Merged

3 tasks

mejiaj commented Nov 21, 2022

View reviewed changes

mejiaj marked this pull request as ready for review November 21, 2022 21:17

mejiaj requested a review from amyleadem November 21, 2022 21:17

amyleadem reviewed Nov 23, 2022

View reviewed changes

mejiaj mentioned this pull request Dec 1, 2022

USWDS-Site - Bug: Remove extra whitespace in identifier links uswds/uswds-site#1928

Closed

2 tasks

mejiaj requested a review from amyleadem December 2, 2022 14:48

amyleadem approved these changes Dec 2, 2022

View reviewed changes

mejiaj requested a review from thisisdano December 2, 2022 17:53

mejiaj added this to the uswds 3.4.0 milestone Dec 21, 2022

thisisdano approved these changes Feb 28, 2023

View reviewed changes

thisisdano merged commit fc31c6e into develop Feb 28, 2023

thisisdano mentioned this pull request Mar 9, 2023

USWDS 3.4.0 #5170

Merged

mejiaj deleted the jm-external-link-wrapping branch July 18, 2023 16:52

Navigation Menu

Uh oh!

USWDS - Links: Fix external link icon issues - #5046

USWDS - Links: Fix external link icon issues#5046
thisisdano merged 6 commits into
developfrom
jm-external-link-wrapping

mejiaj commented Nov 18, 2022 •

edited

Loading

Uh oh!

mejiaj Nov 21, 2022

Uh oh!

mejiaj Nov 21, 2022

Uh oh!

mejiaj Nov 21, 2022

Uh oh!

jhancock532 commented Nov 23, 2022 •

edited

Loading

Uh oh!

amyleadem left a comment •

edited

Loading

Uh oh!

mejiaj commented Nov 30, 2022

Uh oh!

jhancock532 commented Dec 1, 2022

Uh oh!

mejiaj commented Dec 1, 2022

Uh oh!

mejiaj commented Dec 1, 2022

Uh oh!

mejiaj commented Dec 2, 2022

Uh oh!

amyleadem left a comment

Uh oh!

thisisdano left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

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

Uh oh!

Description

Additional information

Link and icon wrapping bug

Safari display bug

Browsers tested

How to test

Uh oh!

mejiaj Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

mejiaj Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

mejiaj Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

jhancock532 commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amyleadem left a comment • 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!

mejiaj commented Nov 30, 2022

Uh oh!

jhancock532 commented Dec 1, 2022

Uh oh!

mejiaj commented Dec 1, 2022

Uh oh!

mejiaj commented Dec 1, 2022

Uh oh!

mejiaj commented Dec 2, 2022

Uh oh!

amyleadem left a comment

Choose a reason for hiding this comment

Uh oh!

thisisdano left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mejiaj commented Nov 18, 2022 •

edited

Loading

jhancock532 commented Nov 23, 2022 •

edited

Loading

amyleadem left a comment •

edited

Loading