Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add parameter to @link-icon to adjust vertical position of icon and different options for $animate parameter #504

Merged
merged 5 commits into from
Oct 8, 2019
Merged

Add parameter to @link-icon to adjust vertical position of icon and different options for $animate parameter #504

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
771c43b
Add parameter to @link-icon to adjust vertical position of icon; fine…
yvonnetangsu Oct 2, 2019
051af57
Use em unit instead of px
yvonnetangsu Oct 2, 2019
373c4bc
Change $animate parameter in @link-icon to take different animation o…
yvonnetangsu Oct 4, 2019
9f70057
Update _link-icon.scss
sherakama Oct 8, 2019
9f94215
Update _link-icon.scss
sherakama Oct 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions core/dist/css/decanter.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion core/src/scss/components/link/_link--external.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
// @see scss/utilities/mixins/link/_link-icon.scss
//
.su-link--external {
@include link-icon(arrow-up-right, 0.6em);
@include link-icon(arrow-up-right, 0.6em, -0.06em);
}
2 changes: 1 addition & 1 deletion core/src/scss/components/link/_link--internal.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
// @see scss/utilities/mixins/link/_link-icon.scss
//
.su-link--internal {
@include link-icon(lock, 0.75em);
@include link-icon(lock, 0.75em, -0.06em);
}
2 changes: 1 addition & 1 deletion core/src/scss/components/link/_link--video.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
// @see scss/utilities/mixins/link/_link-icon.scss
//
.su-link--video {
@include link-icon(video, 0.8em);
@include link-icon(video, 0.8em, -0.2em);
}
7 changes: 4 additions & 3 deletions core/src/scss/utilities/mixins/link/_link-icon.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
@charset "UTF-8";

//
// @link-icon($link-icon, $width: 0.65em, $animate: true)
// @link-icon($icon, $width: 0.65em, $vertical: null, $animate: true)
//
// Display an icon after a link.
//
// $icon {string} - Basename of link icon .svg file (without the .svg suffix).
// $width {string} - Width of icon including unit, e.g. 0.75em, 24px.
// $vertical {string] - Adjustment of vertical position of icon, e.g., '2px' moves icon up 2 pixels, '-0.3em' moves it down 0.3 em.
// $animate {boolean} - Whether or not to animate on hover.
//
// Style guide: Mixins.Link.link-icon
//
@mixin link-icon($icon, $width: 0.65em, $animate: true) {
@mixin link-icon($icon, $width: 0.65em, $vertical: null, $animate: true) {
Copy link
Member

Choose a reason for hiding this comment

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

Technically, changing the order of the parameter input is changing API so in order to not break API, you will need to add it to the end of the mixin parameters. It is much cleaner where it is right now, so if you would like to put this in as a PR to v6 I'll be ok with where it is.

Also, shouldn't the default value for the vertical alignment be -1px as that was the original value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, @sherakama, and thanks for reviewing. When is v6 coming out? Is it after Decanterpalooza 3 this Dec? If so, then it's totally fine that we put this in v6. I just wanted to do this before we work on those new card/hero variants so that the icons are in the right place for those CTA links.

I actually found that a few of those icons don't need any vertical adjustments/margin-bottom so I'd rather have the default be null instead. Some of those with the -1px value look slightly misaligned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, @sherakama, now that I think about it, I'm going to change the $animate parameter since Kerri asked for a few different animation. In that case, maybe I'll keep the original parameter order 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, V6 is planned for release after the next Palooza sprint which is in December. We will likely release a new version in January as we usually have a few items to wrap up after.

text-decoration: none;
@supports (mask-repeat: no-repeat) {
&::after {
@include margin(null 0.3em -1px 0.4em);
@include margin(null 0.3em $vertical 0.4em);
@include size($width);
display: inline-block;
content: '';
Expand Down