feat(NotificationBadge): Add animation for notify#11675
feat(NotificationBadge): Add animation for notify#11675thatblindgeye merged 6 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-11675.surge.sh A11y report: https://patternfly-react-pr-11675-a11y.surge.sh |
There was a problem hiding this comment.
Met with Kayla quickly this morning to confirm expectation for when animations should be triggered. What you have here is essentially the expectation, i.e. we don't bake any specific logic internally, and instead the consumer has to handle logic when setting the hasAnimation prop. What I had done was whip up a quick example where hasAnimation={count % 5 === 0} so that the pf-m-notify class was applied or removed based on the count (no rhyme or reason to this, I just arbitrarily chose that condition).
Just some quick comments below based on the convo she and I had and what I noticed. I think this would be good to add any tests + example and pull out of draft.
packages/react-core/src/components/NotificationBadge/NotificationBadge.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationBadge/NotificationBadge.tsx
Outdated
Show resolved
Hide resolved
afdc234 to
6a14882
Compare
packages/react-core/src/components/NotificationBadge/NotificationBadge.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationBadge/NotificationBadge.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationBadge/NotificationBadge.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationBadge/__tests__/NotificationBadge.test.tsx
Show resolved
Hide resolved
packages/react-core/src/components/NotificationBadge/examples/NotificationBadge.md
Outdated
Show resolved
Hide resolved
6a14882 to
c18ba39
Compare
There was a problem hiding this comment.
the animation itself looks good, just doubling down on the request for a reset animation button and a start or trigger animation button for example purposes(button could reset count to zero if its been triggered a bunch)
c18ba39 to
5b1f5fa
Compare
There was a problem hiding this comment.
😍 this is looking nice, just one quick update below otherwise this is lgtm
packages/react-core/src/components/NotificationBadge/examples/NotificationBadge.md
Outdated
Show resolved
Hide resolved
|
Personally I'd put a button that just increments the count and has it notify each time, maybe a button to reset the count but that seems less important. I just sort of would like to be able to see that you can notify multiple times and having the count appear and disappear because it goes between zero and non-zero sort of obscures that. Also, because the buttons all shift when the count appears, it feels more obtrusive than it really is. But I can be convinced to leave it as is if others are happy. |
5b1f5fa to
fb29d5a
Compare
There was a problem hiding this comment.
Beautiful tintinnabulation! 🔔
What: Closes #10624