Use svg icons as "mask" so their colors could be changed using CSS background-color #426

yvonnetangsu · 2019-06-10T19:04:56Z

READY FOR REVIEW

Summary

Investigate best way to use SVG icons
Investigate the best way to include custom icons into our system #423
Use SVG icons as mask seems to do the trick (except IE). After discussion with Kerri and JB, we think it's ok to just hide the icon in IE.
This is just a test so I've only done work on .su-link--external. If this way of including SVG icon is approved, we'll refactor the other components where SVG icons are used in another PR.

Needed By (Date)

Sooner the better since Kerri/JB is working on the new link components this week.

Urgency

Earlier the better :)

Steps to Test

Pull this branch and compile styleguide
Go to Components - links, and check that the external link variant icon works fine in all browsers except IE, and that the icon is hidden in IE.

Affected Projects or Products

Decanter

Associated Issues and/or People

Investigate the best way to include custom icons into our system #423

sherakama · 2019-06-10T21:37:35Z

core/src/scss/utilities/mixins/link/_external-link.scss

+  }
+
+  // IE only - hides icon.
+  @media screen and (-ms-high-contrast: active), (-ms-high-contrast: none) {


Can we make this a mixin? This seems like something we would want to standardize and have available.

Sounds like a great idea :)

sherakama · 2019-06-10T21:41:19Z

core/src/scss/utilities/mixins/link/_external-link.scss

-    height: 0.65em;
-    width: 0.65em;
+    content: '';
+    mask: url("#{$image-path}/#{$external-link}.svg") no-repeat 0 0;


Browser support for this looks limited: https://caniuse.com/#feat=css-masks
Did you check that this spec meets our compatibility requirements? https://github.com/SU-SWS/decanter/wiki/Compatibility

sherakama · 2019-06-10T21:41:55Z

core/src/scss/utilities/mixins/link/_external-link.scss

-    width: 0.65em;
+    content: '';
+    mask: url("#{$image-path}/#{$external-link}.svg") no-repeat 0 0;
+    mask-size: cover;


This doesn't look well supported: https://developer.mozilla.org/en-US/docs/Web/CSS/mask-size#Browser_compatibility

sherakama

Can you verify that the browser support requirements for Decanter are met?

yvonnetangsu · 2019-06-10T21:45:11Z

Can you verify that the browser support requirements for Decanter are met?

Yeah, I've tested on desktop Firefox, Safari, Chrome, Edge - those are all ok. I'll test it on Android and iOS.

yvonnetangsu · 2019-06-10T21:59:53Z

Can you verify that the browser support requirements for Decanter are met?

Yeah, I've tested on desktop Firefox, Safari, Chrome, Edge - those are all ok. I'll test it on Android and iOS.

@sherakama , I checked a few devices on Browserstack picking not the newest phones on purpose - Galaxy S8 v.7 Chrome, iOS 11 iPhone SE Safari, iOS 10 iPhone 7 Chrome, Pixel 2 Android 8 Firefox and all ok.

sherakama

Thank you for testing. This is GTG then.

yvonnetangsu · 2019-06-10T22:11:52Z

Thank you for testing. This is GTG then.

Cool - I'm writing that IE-only mixin. Should be fairly quick.

* master: Move PR template (#416)

…or below

JBCSU · 2019-06-11T23:15:24Z

Our compatibility guidelines say we will support the latest version of Edge and 1 back. But the mask only seems to work on the latest version of Edge, i.e. 18. It doesn't render on Edge 17 or 16. I'm totally fine with that - just sayin'. Otherwise, (almost) all the device / browser combinations I tested behave as expected. I had trouble with the default (Samsung) browser on a Galaxy S8, but it was not related to this change.

JBCSU

This is a great step forward for us. Thanks!

yvonnetangsu · 2019-06-11T23:22:10Z

Our compatibility guidelines say we will support the latest version of Edge and 1 back. But the mask only seems to work on the latest version of Edge, i.e. 18. It doesn't render on Edge 17 or 16. I'm totally fine with that - just sayin'. Otherwise, (almost) all the device / browser combinations I tested behave as expected. I had trouble with the default (Samsung) browser on a Galaxy S8, but it was not related to this change.

Yeah, I found out about that last night. Though https://caniuse.com/#search=mask says it can be enabled in Edge 17 behind the flag? Any idea what that means? Is that a setting in the browser?

I'm still hoping that this is ok, since Edge is going Chromium soon and I like the ease of being able to change the colors easily. What do you think? 🤔

yvonnetangsu · 2019-06-11T23:27:37Z

Our compatibility guidelines say we will support the latest version of Edge and 1 back. But the mask only seems to work on the latest version of Edge, i.e. 18. It doesn't render on Edge 17 or 16. I'm totally fine with that - just sayin'. Otherwise, (almost) all the device / browser combinations I tested behave as expected. I had trouble with the default (Samsung) browser on a Galaxy S8, but it was not related to this change.

Perhaps we do this in phases. We use this approach for the more decorative icons (e.g., external link), and for components like Main Nav (which it's more important to have the icons visible), we switch to this approach after a newer version of Edge gets pushed out and IE population drops even lower ?

JBCSU · 2019-06-11T23:34:40Z

[...] https://caniuse.com/#search=mask says it can be enabled in Edge 17 behind the flag? Any idea what that means? Is that a setting in the browser?

MS Edge 17 doc says:

EdgeHTML 17 includes experimental support for CSS Masking. The partial implementation introduces the CSS mask-image and mask-size properties. Check the "Enable CSS Masking" flag in about:flags to being experimenting!

No Edge 17 users will do this, so it's effectively unsupported.

I'm still hoping that this is ok, since Edge is going Chromium soon and I like the ease of being able to change the colors easily. What do you think? 🤔

I'm fine with it being only the latest version of Edge. @sherakama, how about you?

yvonnetangsu · 2019-06-11T23:36:21Z

Hey @JBCSU did you close this PR? 👀

* master: 424 default link style (#427) Update package.json (#438) Set up color maps for Decanter and refactor main nav colors to refer to color maps (#420)

… link

JBCSU · 2019-06-12T00:35:24Z

core/src/scss/utilities/variables/core/_core-color-maps.scss

@@ -37,10 +37,10 @@ $core-colors: (
  text--high-contrast:          #000,
  text--reverse:                $color-white,

+  link:                         $color-bright-blue,
+  link--hover:                  $color-black,


I chose to replace link and link--hover with link-inline and link-inline--hover to be more explicit about the application. I don't want to have both, so let's decide. I prefer the -inline variants, in which case please use those. But if you feel strongly that link and link--hover are better, then please delete the ones I added and change all references to the -inline map entries to match

I like having a clean default link color without any modifiers since we have clean bg, text colors. Also, the -inline makes me wonder if it only applies to display:inline links. For example, the links in the vertical link list will probably take on this default link color, but it might be a bit strange if I apply the color link-inline even though the link might be a block element.

Ok, then please delete the -inline options and update all references to them to refer to the shorter names.

Done! Added a link--alt and link--alt-hover for cases like the secondary button link style. I feel like the bright red link will appear on some sites in some other ways so I make that the link--alt color.

Ok. I introduced an error in the doc for @function color(), so I fixed that.

…secondary button for example)

JBCSU

I think this is GTG.

yvonnetangsu · 2019-06-12T01:51:25Z

I think this is GTG.

Yay, merging before you changed your mind 😂

* master: Use svg icons as "mask" so their colors could be changed using CSS background-color (#426) 424 default link style (#427) Update package.json (#438)

* master: Use svg icons as "mask" so their colors could be changed using CSS background-color (#426)

Hides icon in IE

e8273b5

yvonnetangsu requested review from sherakama and JBCSU June 10, 2019 19:05

Use Bourbon @SiZe mixin for width and height and rearrange properties

abfc56d

sherakama reviewed Jun 10, 2019

View reviewed changes

sherakama requested changes Jun 10, 2019

View reviewed changes

sherakama approved these changes Jun 10, 2019

View reviewed changes

yvonnetangsu added 4 commits June 10, 2019 15:32

Add helper mixin @ie-only for writing styles targeting only IE

a4c45dc

Merge branch 'master' into 423-svg-icon-test

63d8622

* master: Move PR template (#416)

Use @supports for browser detection instead

0ad0ab0

Better browser targeting - include Edge 18 and + but exclude Edge 17 …

983a85d

…or below

yvonnetangsu added the Discovery label Jun 11, 2019

JBCSU approved these changes Jun 11, 2019

View reviewed changes

JBCSU closed this Jun 11, 2019

JBCSU reopened this Jun 11, 2019

yvonnetangsu added 2 commits June 11, 2019 17:07

Merge branch 'master' into 423-svg-icon-test

328db3a

* master: 424 default link style (#427) Update package.json (#438) Set up color maps for Decanter and refactor main nav colors to refer to color maps (#420)

Merge master and tweak color map; add proper focus state for external…

a01bc75

… link

JBCSU requested changes Jun 12, 2019

View reviewed changes

yvonnetangsu and others added 2 commits June 11, 2019 18:29

Replace link-inline with link in $core-colors and add link--alt (for …

23ead0f

…secondary button for example)

fix doc on @function color()

0e0227f

JBCSU approved these changes Jun 12, 2019

View reviewed changes

yvonnetangsu merged commit e6d2f8a into master Jun 12, 2019

yvonnetangsu deleted the 423-svg-icon-test branch June 12, 2019 01:53

yvonnetangsu added a commit that referenced this pull request Jun 12, 2019

Merge branch 'master' into 222-masthead

c353fcd

* master: Use svg icons as "mask" so their colors could be changed using CSS background-color (#426) 424 default link style (#427) Update package.json (#438)

JBCSU added a commit that referenced this pull request Jun 12, 2019

Merge branch 'master' into 436-437-cards

7eecd0c

* master: Use svg icons as "mask" so their colors could be changed using CSS background-color (#426)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use svg icons as "mask" so their colors could be changed using CSS background-color #426

Use svg icons as "mask" so their colors could be changed using CSS background-color #426

yvonnetangsu commented Jun 10, 2019 •

edited

Loading

sherakama Jun 10, 2019

yvonnetangsu Jun 10, 2019

sherakama Jun 10, 2019

sherakama Jun 10, 2019

sherakama left a comment

yvonnetangsu commented Jun 10, 2019

yvonnetangsu commented Jun 10, 2019

sherakama left a comment

yvonnetangsu commented Jun 10, 2019

JBCSU commented Jun 11, 2019

JBCSU left a comment

yvonnetangsu commented Jun 11, 2019

yvonnetangsu commented Jun 11, 2019

JBCSU commented Jun 11, 2019

yvonnetangsu commented Jun 11, 2019

JBCSU Jun 12, 2019

yvonnetangsu Jun 12, 2019

JBCSU Jun 12, 2019

yvonnetangsu Jun 12, 2019

JBCSU Jun 12, 2019

yvonnetangsu Jun 12, 2019

JBCSU left a comment

yvonnetangsu commented Jun 12, 2019

Use svg icons as "mask" so their colors could be changed using CSS background-color #426

Use svg icons as "mask" so their colors could be changed using CSS background-color #426

Conversation

yvonnetangsu commented Jun 10, 2019 • edited Loading

READY FOR REVIEW

Summary

Needed By (Date)

Urgency

Steps to Test

Affected Projects or Products

Associated Issues and/or People

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherakama left a comment

Choose a reason for hiding this comment

yvonnetangsu commented Jun 10, 2019

yvonnetangsu commented Jun 10, 2019

sherakama left a comment

Choose a reason for hiding this comment

yvonnetangsu commented Jun 10, 2019

JBCSU commented Jun 11, 2019

JBCSU left a comment

Choose a reason for hiding this comment

yvonnetangsu commented Jun 11, 2019

yvonnetangsu commented Jun 11, 2019

JBCSU commented Jun 11, 2019

yvonnetangsu commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBCSU left a comment

Choose a reason for hiding this comment

yvonnetangsu commented Jun 12, 2019

yvonnetangsu commented Jun 10, 2019 •

edited

Loading