-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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.
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
feat(snack-bar): align with 2018 material design spec #12634
Conversation
animate(`${AnimationDurations.ENTERING} ${AnimationCurves.DECELERATION_CURVE}`)), | ||
state('void', style({opacity: 0, transform: 'scale(0.8)'})), | ||
state('visible', style({transform: 'scale(1)'})), | ||
transition('* => visible', animate('150ms cubic-bezier(0, 0, 0.2, 1)', style({ |
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.
Is there a reason to move away from using the AnimationDurations
?
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.
I based the timing on the new tooltip animation which looks very similar to this one.
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.
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.
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.
LGTM
What's the contrast ratio on the text color to background now? Just from the screenshot it doesn't look great |
Yeah, it's not great, but it's what's in the designs. This is the reason why I kept the old text color for the dark-themed snackbar where the contrast was even worse. |
ae6e894
to
8d46e7e
Compare
Is the ratio an |
I haven't looked at the exact ratio and I'm not sure what I'm supposed to use to measure it either. |
It's based on the text color and the background color. I always use this tool: https://contrast-ratio.com/ |
According to the tool, it's an AAA contrast (7.15). The text color is |
Ah, that's perfect then. |
animate(`${AnimationDurations.EXITING} ${AnimationCurves.ACCELERATION_CURVE}`)), | ||
transition('void => visible-top, void => visible-bottom', | ||
animate(`${AnimationDurations.ENTERING} ${AnimationCurves.DECELERATION_CURVE}`)), | ||
state('void', style({opacity: 0, transform: 'scale(0.8)'})), |
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.
Does it make sense to have styles in the transition()
calls?
Could this just be:
snackBarState: trigger('state', [
state('void', style({
opacity: 0,
transform: 'scale(0.8)'
})),
state('visible', style({
transform: 'scale(1)',
opacity: 1,
})),
transition('* => visible', animate('150ms cubic-bezier(0, 0, 0.2, 1)')),
transition('* => void', animate('75ms cubic-bezier(0.4, 0.0, 1, 1)')),
])
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.
Not exactly. The code above will cause it to scale away for the exit animation, but we only want it to fade out. This means that we need a style({opacity: 0})
on the * => void
. I've updated it with some tweaks.
8d46e7e
to
4e9c779
Compare
@@ -94,11 +94,11 @@ export class MatSnackBarContainer extends BasePortalOutlet implements OnDestroy | |||
onAnimationEnd(event: AnimationEvent) { | |||
const {fromState, toState} = event; | |||
|
|||
if ((toState === 'void' && fromState !== 'void') || toState.startsWith('hidden')) { | |||
if (toState === 'void' && fromState !== 'void') { |
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.
We should not need to check the from state. If we don't take action in cases where we go to void
from void
then it results in leaving the snackbar-container in the DOM indefinitely.
We should be able to just to if (toState === 'void') {
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.
This isn't something I introduced in this PR, I just had to rework the condition. If I remember correctly, the case where we could get the fromState
and toState
to both be void
was on enter, which shouldn't be an issue for this PR.
Here's the commit for reference: crisbeto@8e77261
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.
Hmm, in my testing I didn't see any issue with removing the fromState
check. I think the difference now is that we are using void
instead of hidden
for our hiding state.
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.
I'm inclined to keep it just because it shouldn't hurt anything and these changes are dangerous enough as they are.
4e9c779
to
7ce083d
Compare
Aligns the snack bar component with the latest Material Design spec.
7ce083d
to
7d08aff
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Aligns the snack bar component with the latest Material Design spec.