feat(Alert): added animations

[thatblindgeye]

What: Closes #10440

Some specific input from design: does the animation on removal go too quickly? I'm using (I believe) the token value for the transition used in Core (100ms). When I had originally tried hardcoding a timeout I used 200ms (so 200ms from the Alert being clicked to remove/timed out to it actually being removed from the DOM) and that looked a tad nicer IMO, but using an arbitrary length of time like that may not be great + just *2 the token value may not be great either if the default value from Core/overrides gets too be too long

Additional issues:

[patternfly-build]

Preview: https://patternfly-react-pr-11495.surge.sh

A11y report: https://patternfly-react-pr-11495-a11y.surge.sh

[tlabaj]

Should the demo be updated?

[srambach]

I can see the alert gets the .pf-m-offstage-right class, but the transition doesn't run. If I add the class manually, it runs. If I remove the class after clicking the dismiss button, it transitions back onto the screen.
I'm not sure why adding the class wouldn't run the transition - any ideas based on how you are adding the class?

[thatblindgeye]

Should the demo be updated?

@tlabaj possibly a question for design? I wasn't sure if we wanted all of our AlertGroup instances to have animations on by default or not. If we do, we could update the new prop default to be true

I can see the alert gets the .pf-m-offstage-right class, but the transition doesn't run. If I add the class manually, it runs. If I remove the class after clicking the dismiss button, it transitions back onto the screen.
I'm not sure why adding the class wouldn't run the transition - any ideas based on how you are adding the class?

@srambach I think the issue is that the Alert gets removed too quickly for the full transition to be seen. If you look at the left edge of an Alert as you press the "close" button for it, you should be able to see that it starts to transition to the right, it just derenders so quickly. I could fiddle with the timeout between that offstage-right class getting applied and the alert actually being removed.

[thatblindgeye]

Notes from animations review meeting:

• Make animations true by default
• fiddle with the timing when removed and/or look into transitionend event (look at https://codepen.io/mcoker/pen/qBedQzo for inspiration)

[mfrances17]

LGTM, code looks good, tested fine, nice work.

This has nothing to do with your code or implementation but as an end user I would expect animations on alert close to do the exact opposite direction of on open. So if it the animation of the alert makes it move down from top, the animation on close should have it move up back to top. Likewise, if it animates in from left on open, it should animate to right on close. In other words, returning to the place from which it came. :) But this is a comment on the design not the implementation, so I digress.

[thatblindgeye]

Met with @mcoker and @srambach yesterday afternoon to discuss this PR, specifically using the transitionend event when alerts are removed rather than using a CSS token+setTimeout.

The latest commit is an attempt at doing that as well as updating animations to be enabled by default (cc @andrew-ronaldson ). Some notes:

• We needed to have logic to call applicable callbacks (onTimeout and onClose) if animations are disabled at the component level, as well as when animations are enabled at the component level
• We needed to ensure those same callbacks were called appropriately when animations were enabled at the component level, but a user has a reduced motion setting enabled (this is why there's conditional logic checking for the matchesMedia method against specific transitions)
  • Part of the reason for the above is that when animations are running, we need to wait for the animation where remaining alerts "slide up" when one is removed -- otherwise the remaining alerts just snap into place abruptly when another alert is removed)
• We needed to prevent callbacks from ebing called when they're not supposed to, e.g. onClose when an alert times out or onTimeout when an alert is manually closed

[tlabaj]

Since hasAnimations is true by default. I would say this examples is not technically needed correct? I wonder if we can combine the docs from this with another example. @edonehoo I would be interested in your thoughts here.

[edonehoo]

Hmm I would agree that this example doesn't seem necessary if hasAnimations is the default behavior.

Or I'm wondering what we're trying to explain via this example that we aren't doing in previous examples? Is it this part? "When using animations, each alert must have a unique id or key passed to it."

We could add an asterisk/update the description of id in the props table saying that it's required when using animations? And/or add that to the hasAnimations description

[kmcfaul]

Do we need to account for RTL with alert groups and the animations - like have an offstageLeft?

Also, right now the full page demo with toast alerts doesn't seem to work at all when you flip on RTL (alerts do not pop up and the notification drawer doesn't open/close until you turn off RTL again).

[tlabaj]

Great Question!

[srambach]

patternfly/patternfly#7365 to address RTL.

[thatblindgeye]

Latest commit 8 includes some refactoring of how to handle the transitionend logic - rather than adding eventlisteners to the document (would create a lot of noise), logic is passed to the onTransitionEnd handler of the AlertGroupItem.

I kept the hasAnimations prop, as that may be necessary for consumers to pass as false for testing purposes. The class modifier killswitch didn't work from what I had tried in getting this test to pass. You can see what we had to do to get one of the AlertGroup tests to pass. Keep in mind that this might only be an issue if a consumer is specifically testing the Alert's onClose callback while passing that Alert to an AlertGroup (like how we are in the test). If the consumer just has a standalone Alert, their tests should be fine.

Also a note: the original implementation where I had used timeouts instead of leveraging the transitionend (from the first commit), also would've required an update to the AlertGroup test (albeit slightly simpler).

[srambach]

🚨 good to go from my perspective!

[kmcfaul]

Shouldn't this be opt-in rather than opt-out? It looks like at least one of our cypress demos needed an update to turn animations off in this PR, so I'm concerned this may be breaking.

[kaylachumley]

not sure if this should be opt-in or not! ^ wdyt? @thatblindgeye

[kmcfaul]

Everything else lgtm so I'll leave an approval, but I would love some feedback about my above comment about the current opt-out pattern of the new prop though.

I think we should probably make this and other animations opt-in personally, until V7 where we can make them default.

[kaylachumley]

lgtm from the design side!

[tlabaj]

Everything else lgtm so I'll leave an approval, but I would love some feedback about my above comment about the current opt-out pattern of the new prop though.

I think we should probably make this and other animations opt-in personally, until V7 where we can make them default.

@thatblindgeye Can we just turn off the animations internally? If the animations "kill switch" style is applied in the DOM we turn animations off in Alerts? That way the kill switch will control turning off all animations.

[kmcfaul]

LGTM.

The one last thing I noticed is that we updated our demos to put new alerts at the beginning on the alert array instead of appending them, so that has some implication for users (just testing on the updated preview with the animations on and that change reverted - because the animation is triggered on the first item, additional alerts appended to the array will be added but not have the animation triggered). But since it's now opt in, I think that's part of opting in as well as updating tests.

[thatblindgeye]

Followup to discussions from today: #11684