Links: Use add-color-icon mixin with currentColor for external link#4297
Conversation
mejiaj
left a comment
There was a problem hiding this comment.
Great work, just a question regarding currentColor in _link.scss.
| usa-icons-bg/launch--gray-5, | ||
| usa-icons-bg/launch--white | ||
| ); | ||
| @include external-link("base-darker"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Should the expected value be something other than
currentColorbased 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
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?
| @include external-link("base-darker"); | |
| @include external-link($contrast-bg: "base-darker"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see. Initially thought it was defining the background to set the accessible link color. Because I tried setting
$contrast-bgand the background toprimary-vividand 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.
There was a problem hiding this comment.
In 872dc9e, I updated to use the named argument syntax just to make it extra clear what's being passed.
|
@thisisdano I just noticed this line uswds/src/stylesheets/core/mixins/_icon.scss Line 126 in 1a510dc If we allow users to set the path, shouldn't we remove that #{$path}/#{$filename-base}.svgIf so, I can create a separate issue for this. |
Clarify intention of argument as defining anticipated background against which to constrast
There was a problem hiding this comment.
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
|
The only custom color icon we need to maintain is |
|
Otherwise, this looks good to me! |
Ah, good catch! I missed the concatenation in the icon helper. Restored in bbf2d30.
|
Description
Refactors external link styling to use
add-color-iconmixin, usingcurrentColorto 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:
npm testand make sure the tests for the files you have changed have passed.