feat(button): add animation for hover and clicked#6675
Conversation
|
Preview: https://patternfly-pr-6675.surge.sh A11y report: https://patternfly-pr-6675-a11y.surge.sh |
|
@andrew-ronaldson should this same animation also be on menu toggles? |
Yes I think so. |
There was a problem hiding this comment.
sweet moves, ace!
| transition-timing-function: var(--#{$button}--TransitionTimingFunction); | ||
| transition-duration: var(--#{$button}--TransitionDuration); | ||
| transition-property: background-color; |
There was a problem hiding this comment.
Just a nit and you may know this but the shorthand version would be
transition: background-color var(--#{$button}--TransitionDuration) var(--#{$button}--TransitionTimingFunction);
There was a problem hiding this comment.
Since the property isn't a variable I wasn't sure if the shorthand version was advisable or not? I can change it if you think it's better.
There was a problem hiding this comment.
@mcoker I realized color needed to be transformed as well, so I left this as is since the shorthand doesn't take multiple properties (unless we use all)
There was a problem hiding this comment.
LGTM! Left a few nit comments.
Also maybe this question has already been asked or is obvious, but in figma the annotations say "on button click...". Just want to point out that for the button, we have "clicked" styles for the .pf-m-clicked variation, but that might be different from just clicking on a button. The .pf-m-clicked variation is for something like a button that's used to open a drawer. We'll apply the clicked variation to indicate that the drawer is expanded, and remove it when the drawer is closed. Just plain ol' clicking on a button triggers something called :active, which happens when you hold down your mouse button (or whatever pointer device or the enter/space key on the keyboard). Here's a codepen showing an :active transition - https://codepen.io/mcoker/pen/WNBROpE. I don't know that it means anything in terms of this PR, since the hover/focus and clicked transitions are all the same, but wanted to make sure we were referring to the same thing.
| --#{$button}--hover--TransitionDuration: var(--pf-t--global--motion--duration--fade--default); | ||
| --#{$button}--hover--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); |
There was a problem hiding this comment.
Do we need hover (and clicked) vars? If the goal is just to smooth out the background/border/text color changes that happen as either the user interacts with the button, or the button changes states programmatically, it seems like a base transition for those properties should be good enough, and whatever state changes happen will all get that change.
| transition-timing-function: var(--#{$button}--TransitionTimingFunction); | ||
| transition-duration: var(--#{$button}--TransitionDuration); | ||
| transition-property: border-color, border-width, color; |
There was a problem hiding this comment.
We don't declare anything in content for ::after, so I don't think you need color here.
And since it's ::after and the vars used are the same as the parent, assuming you'd just add border-color, border-width to .#{$button}'s transition-property if these styles existed on that selector, I might write it this way - no downsides that I can think of?
| transition-timing-function: var(--#{$button}--TransitionTimingFunction); | |
| transition-duration: var(--#{$button}--TransitionDuration); | |
| transition-property: border-color, border-width, color; | |
| transition: inherit; // inherits everything | |
| transition-property: border-color, border-width; // changes transition-property |
An aside, color isn't showing up in the preview for some reason, but it's there locally when I pull down your branch, so I'm assuming it's just a surge glitch 🤷♂️
|
Oh I meant to leave one more comment. In terms of allowing users to theme/customize the button animations, I think this PR is good, but it looks to assume the following which I just wanted to confirm is 👍👍
|
There was a problem hiding this comment.
Looks good! Just one question 👍
| transition-timing-function: inherit; | ||
| transition-duration: inherit; |
There was a problem hiding this comment.
Just to confirm - since the initial value of transition-property is "all", leaving that out here will set that. Was that intended? If not, transition: inherit; should do the trick.
There was a problem hiding this comment.
eh, not really 😄 I'll fix it.
|
🎉 This PR is included in version 6.0.0-alpha.146 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #6589
There is no reduced-motion variation for this one.
Figma specs: https://www.figma.com/design/VMEX8Xg2nzhBX8rfBx53jp/branch/YIxfglww1fRdQGclApv1nm/PatternFly-6%3A-Components?m=auto&node-id=23613%3A1181&t=yaDSp7SyRyKeEd2A-1