Links: Use add-color-icon mixin with currentColor for external link

[aduth]

Description

Refactors external link styling to use add-color-icon mixin, using currentColor to match link text, avoiding the need for multiple color variations of the icon, and resolving an issue with visited links where the icon color does not match the color of the link (previously reported at #2185).

Additional information

Open question: Should the existing SVG files should be kept around for backward-compatibility, or is it reasonable to remove them since they're now unused in the project?

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.

[mejiaj]

Great work, just a question regarding currentColor in _link.scss.

[mejiaj]

Should the expected value be something other than currentColor based on the SCSS below?

uswds/src/stylesheets/core/mixins/_icon.scss

Line 142 in 837f00f

background-color: if($color == currentColor, $color, color($color));

[aduth]

Should the expected value be something other than currentColor based on the SCSS below?

The value here is forwarded along as the $contrast-bg value for add-color-icon's IE11 fallback. I selected it based on it being the background color for .usa-dark-background:

uswds/src/stylesheets/elements/typography/_content.scss

Lines 38 to 40 in 1908d13

	.usa-dark-background {
	@include add-knockout-font-smoothing;
	background-color: color("base-darker");

Maybe it would be clearer if I updated this to be a keyword parameter, so that it's not confused with the color of the icon itself?

Suggested change

	@include external-link("base-darker");
	@include external-link($contrast-bg: "base-darker");

[mejiaj]

The value here is forwarded along as the $contrast-bg value for add-color-icon's IE11 fallback. I selected it based on it being the background color for .usa-dark-background:

I see. Initially thought it was defining the background to set the accessible link color. Because I tried setting $contrast-bg and the background to primary-vivid and was expecting the link color to change based on the new background color.

[aduth]

I see. Initially thought it was defining the background to set the accessible link color. Because I tried setting $contrast-bg and the background to primary-vivid and was expecting the link color to change based on the new background color.

As I see it, the expectation with these changes is that the icon follows whatever text color happens to be applied to the region, and it would be expected the text color contrast is assigned appropriately. The argument being passed here would only exist to maintain the existing behavior for the IE11 fallback of showing a white icon on dark background, and black icon on light background.

I'm not super familiar with the contrasting color behavior, so let me know if this isn't behaving the way you'd expect and if I should change it.

[aduth]

In 872dc9e, I updated to use the named argument syntax just to make it extra clear what's being passed.

[mejiaj]

@thisisdano I just noticed this line

uswds/src/stylesheets/core/mixins/_icon.scss

Line 126 in 1a510dc

$mask-props: url("#{$path}/usa-icons/#{$filename-base}.svg") no-repeat center /

If we allow users to set the path, shouldn't we remove that usa-icons part? So it'd be:

#{$path}/#{$filename-base}.svg

If so, I can create a separate issue for this.

[mejiaj]

Looks good to me. Regarding your question:

Open question: Should the existing SVG files should be kept around for backward-compatibility, or is it reasonable to remove them since they're now unused in the project?

Yes, they're included for compatibility. Moving forward we're not going to be supporting IE11. But currently the currect icon is showing in IE11. Whether it's okay to remove them or not is a question for @thisisdano.

2.12.1 - IE11

Current change - IE11

[thisisdano]

The only custom color icon we need to maintain is launch--white.svg — this variant is required for IE11 when the colored icon appears on a dark background. IE11 will use the primary (black) icon as a fallback for light backgrounds and the white variant for dark backgrounds. The other variants are used exclusively for link CSS. We can make a note that we've removed the other color variants, but I think it is safe to remove them.

[thisisdano]

Otherwise, this looks good to me!

[aduth]

The only custom color icon we need to maintain is launch--white.svg — this variant is required for IE11 when the colored icon appears on a dark background. IE11 will use the primary (black) icon as a fallback for light backgrounds and the white variant for dark backgrounds. The other variants are used exclusively for link CSS. We can make a note that we've removed the other color variants, but I think it is safe to remove them.

Ah, good catch! I missed the concatenation in the icon helper. Restored in bbf2d30.

Before	After