-
Notifications
You must be signed in to change notification settings - Fork 371
chore(Alert): updated examples to enable animations #11705
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
chore(Alert): updated examples to enable animations #11705
Conversation
Preview: https://patternfly-react-pr-11705.surge.sh A11y report: https://patternfly-react-pr-11705-a11y.surge.sh |
import LaptopIcon from '@patternfly/react-icons/dist/esm/icons/laptop-icon'; | ||
import buttonStyles from '@patternfly/react-styles/css/components/Button/button'; | ||
|
||
Micro-animations have been added for `Alert` components within an `AlertGroup`, which can be seen on all examples and demos where alerts are dynamically added. By default this is opt-in, as enabling animations may require updates to tests. You can pass the `hasAnimations` property to enable or disable the animations as needed. Additionally, with these animations enabled, we recommend you ensure that dynamically added alerts are prepended to a list of alerts, rather than appended to the end of it. |
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.
Micro-animations have been added for `Alert` components within an `AlertGroup`, which can be seen on all examples and demos where alerts are dynamically added. By default this is opt-in, as enabling animations may require updates to tests. You can pass the `hasAnimations` property to enable or disable the animations as needed. Additionally, with these animations enabled, we recommend you ensure that dynamically added alerts are prepended to a list of alerts, rather than appended to the end of it. | |
Micro animations have been added for `<Alert>` components within an `<AlertGroup>`. By default, you must opt into animations, since they can require updates to tests. To enable or disable animations as needed, use the `hasAnimations` property. With animations enabled, we recommend you ensure that dynamically-added alerts are prepended to a list of alerts, rather than appended to the end of it. | |
Micro animations are turned on for all examples and demos where alerts are dynamically added. |
just a little moving things around mainly. I think it makes sense to include this for alert, since we have motion in all examples even though its opt in!
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.
🥇
One minor thing I noticed which can be left as is or explored in a follow up is that in the timeout alert example, if you manually dismiss the alert with the button instead of letting it time out the close animation doesn't play. |
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: Closes #11684
@edonehoo @andrew-ronaldson wdyt about the verbiage above the examples? More or less using the copy from the release highlights, but curious if you think it's worth including/helpful here.
Additional issues: