Skip to content
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.

Sign up for GitHub

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

Jump to bottom

D8CORE-1623: Adding a mixin and icons for a simple icon before dis… #690

Merged
merged 3 commits into from
Oct 6, 2020
Merged

D8CORE-1623: Adding a mixin and icons for a simple icon before dis… #690

merged 3 commits into from
Oct 6, 2020

Conversation

jenbreese
Copy link
Contributor

READY FOR REVIEW

Summary

  • This adds a mixin for icons only in the same way we do for our linked-icons.
  • This has many new icons that the design teams have collaborated on. Most are only SVG but there are a few PNG versions.

Needed By (Date)

  • Soon

Urgency

  • This is very important to the work on the 1.3.0 release of accelerants SWS is working on.

Steps to Test

  1. Pull in this change.
  2. Try out the new icon mixin.

Affected Projects or Products

  • It is important to the accelerant work of SWS.

Associated Issues and/or People

  • D8CORE-1623 - Events accelerant
  • This is a mixin for icons only. It uses the same method as the icon-link. However there is no animation or hover states on the icons. This was discussed with @kerri-augenstein and Ishi. If a hover or animation is needed since it is part of a link, it makes more sense to use the icon-link mixin. It does have the same problem found with the link-icon mixin that IE 11 doesn't work. However, IE11 is not extensively supported.
  • @kerri-augenstein & @sherakama

See Also

@sherakama sherakama temporarily deployed to Tugboat June 4, 2020 16:28 Destroyed
yvonnetangsu
yvonnetangsu requested changes Jun 4, 2020
View reviewed changes
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pull this in later and actually test out the mixin, but added a couple comments.

core/src/scss/utilities/mixins/icons-logos/_icon.scss Outdated Show resolved Hide resolved
core/src/scss/utilities/mixins/icons-logos/_icon.scss Outdated Show resolved Hide resolved
@sherakama sherakama temporarily deployed to Tugboat June 4, 2020 21:11 Destroyed
sherakama
sherakama reviewed Jun 5, 2020
View reviewed changes
core/src/scss/utilities/mixins/icons-logos/_icon.scss Outdated
@include size($width);
display: inline-block;
content: '';
mask: url("#{$su-image-path}/#{$icon}.svg") no-repeat 0 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better for us to not assume path but to pass in the whole path to the $icon param as it could open this up for others to use their own bucket of icons.

@sherakama
Copy link
Member

This is GTG once the change to the image path in the mixin has been applied. Please move the image path variable into the mixin's parameters.

sherakama
sherakama approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG

@sherakama sherakama changed the title D8CORE-1623-v2: Adding a mixin and icons for a simple icon before dis… D8CORE-1623: Adding a mixin and icons for a simple icon before dis… Oct 6, 2020
@sherakama sherakama merged commit 92ef633 into master Oct 6, 2020
@sherakama sherakama deleted the D8CORE-1623-v2 branch October 6, 2020 05:49
yvonnetangsu added a commit that referenced this pull request Oct 8, 2020
@yvonnetangsu
Merge branch 'master' into modular-spacing-map
a11bdb7 
* master:
  D8CORE-1261: Revised table styles. (#617)
  Create auto-label action (#763)
  Add stale action. (#762)
  D8CORE-1623: Adding a mixin and icons for a simple icon before dis… (#690)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yvonnetangsu yvonnetangsu yvonnetangsu approved these changes

@sherakama sherakama sherakama approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jenbreese @sherakama @yvonnetangsu