8000 feat(Button): add notify modifier to button for microanimation by srambach · Pull Request #7368 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

feat(Button): add notify modifier to button for microanimation#7368

Merged
mcoker merged 3 commits intopatternfly:mainfrom
srambach:6601-notification-bell-animation
Feb 25, 2025
Merged

feat(Button): add notify modifier to button for microanimation#7368
mcoker merged 3 commits intopatternfly:mainfrom
srambach:6601-notification-bell-animation

Conversation

@srambach
Copy link
Member

This is for the notification badge part of #6601

A modifier .pf-m-notify gets added to a .pf-m-button in order to cause the icon portion of the button to have the ringing motion.

patternfly/patternfly-react#10624 will need to add the class, wait for the animation to complete, and then remove it, and also manage if a new notification arrives before the animation finishes from the previous notification.

This also requires React to update the notification badge to put the icon into the icon container rather than the text container, following core's structure.

@patternfly-build
Copy link
Collaborator
patternfly-build commented Feb 25, 2025

@srambach srambach requested a review from sg00dwin February 25, 2025 20:18
@srambach
Copy link
Member Author

For convenience - here's the animation on the top of a demo page.

2025-02-25_15-20-10.mp4

Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

It's good as-is but left a few nit comments in case you wanted to change a couple of things I noticed.

Comment on lines +282 to +283
--#{$button}__icon--AnimationDuration--notify: var(--pf-t--global--duration--600);
--#{$button}__icon--AnimationTimingFunction--notify: var(--pf-t--global--motion--timing-function--default);
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, this is how we would normally write out this var. I'm cool leaving it the way you have it, since we have an identifier as --notify on the end. WDYT?

Suggested change
--#{$button}__icon--AnimationDuration--notify: var(--pf-t--global--duration--600);
--#{$button}__icon--AnimationTimingFunction--notify: var(--pf-t--global--motion--timing-function--default);
--#{$button}--m-notify__icon--AnimationDuration--notify: var(--pf-t--global--duration--600);
--#{$button}--m-notify__icon--AnimationTimingFunction--notify: var(--pf-t--global--motion--timing-function--default);

Comment on lines +713 to +714
animation-duration: var(--#{$button}__icon--AnimationDuration--notify);
animation-timing-function: var(--#{$button}__icon--AnimationTimingFunction--notify);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you made the other suggestion, we'd also want to update this

Suggested change
animation-duration: var(--#{$button}__icon--AnimationDuration--notify);
animation-timing-function: var(--#{$button}__icon--AnimationTimingFunction--notify);
animation-duration: var(--#{$button}--m-notify__icon--AnimationDuration--notify);
animation-timing-function: var(--#{$button}--m-notify__icon--AnimationTimingFunction--notify);

Comment on lines +759 to +761
100% {
transform: rotate(0);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit but since it starts at rotate(0), you shouldn't need this last part

Suggested change
100% {
transform: rotate(0);
}

@mcoker mcoker merged commit 7ab07c4 into patternfly:main Feb 25, 2025
4 checks passed
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.2.0-prerelease.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0