-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(chips): Add delay to filter chip checkmark #2804
Conversation
Add delay to transition animation so checkmark does not appear behind text. Add speed up to leading icon to brige gap between icons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM but I'm gonna add another reviewer just to double check
@@ -118,10 +118,12 @@ | |||
.mdc-chip__icon--leading { | |||
transition: opacity $mdc-chip-opacity-animation-duration linear; | |||
opacity: 1; | |||
transition-delay: -50ms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here to explain what's this line is for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Codecov Report
@@ Coverage Diff @@
## master #2804 +/- ##
=======================================
Coverage 98.42% 98.42%
=======================================
Files 98 98
Lines 4203 4203
Branches 534 534
=======================================
Hits 4137 4137
Misses 66 66 Continue to review full report at Codecov.
|
@@ -117,10 +117,16 @@ | |||
|
|||
.mdc-chip__icon--leading { | |||
transition: opacity $mdc-chip-opacity-animation-duration linear; | |||
|
|||
// Speed up delay to bridge gap between leading icon and checkmark transition. | |||
transition-delay: -50ms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this negative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By delaying the transition of the checkmark it also delayed the appearance of the leading icon, and so I used the transition-delay feature to actually speed it up to bridge the gap from checkmark fading out and leading icon fading in. Helps the transition feel/look more natural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah my bad! I thought this was the transition-duration
.
Since these numbers are important, I'd move them to the _variables.scss
file.
Once that's in, I'll LGTM it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
packages/mdc-chips/_variables.scss
Outdated
@@ -28,6 +28,8 @@ $mdc-chip-trailing-icon-hover-opacity: .62; | |||
$mdc-chip-trailing-icon-focus-opacity: .87; | |||
$mdc-chip-leading-icon-size: 20px; | |||
$mdc-chip-trailing-icon-size: 18px; | |||
$mdc-chip-leading-icon-speedup: -50ms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this name to $mdc-chip-leading-icon-delay
and move the comment about why it's negative to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Fixes #2384
Add delay to transition animation so checkmark does not appear behind text. Add speed up to leading icon to brige gap between icons.