Skip to content
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

DS-692 Animation Tool Set #2419

Merged
merged 64 commits into from Feb 25, 2022
Merged

Conversation

MarcinMr
Copy link
Collaborator

@MarcinMr MarcinMr commented Jan 19, 2022

Jira

https://pegadigitalit.atlassian.net/browse/DS-692

Summary

Animated classes with keyframes were added to packages/global/styles/animations.

Details

There were created fade, bounce, zoom (ins/outs) combined with slide effects (top, bottom, left, right) + modifier classes controlling the cascading appearance of elements and delays options.

fade

  • fade in / out / in-up, out-up / in-down, out-down / in-left, out-left / in-right, out-right

zoom

  • zoom in / out / in-up, out-up / in-down, out-down / in-left, out-left / in-right, out-right

bounce

  • bounce in / out / in-up, out-up / in-down, out-down / in-left, out-left / in-right, out-right

Test pages were created in the tests/animations folder for experimenting.

How to test

Check if animations work properly by adding specific classes to the elements considered to be animated. Make sure the performance of animating elements is sufficient.

@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Jan 19, 2022
@colbytcook colbytcook requested a deployment to feature/DS-692-animation-tool-set--branch-preview January 19, 2022 16:19 Abandoned
@MarcinMr
Copy link
Collaborator Author

MarcinMr commented Jan 19, 2022

@mikemai2awesome

Could you please have the first glimpse of that? I created a few fades classes that can be added to the elements that wanna be animated.

It isn't easy to make the performance efficient on every browser engine. I used that page -> https://csstriggers.com/ to learn about how different browsers behave with CSS triggers. The main rule is not to animate anything which triggers layout for example: padding, margins, width, etc. And animate only those which trigger the "composite" (the safest way to not affect the performance in a bad way).

So in our case, I used opacity, and transform which are the best properties for animations. But... not for every browser. For example, changing transform and opacity on the browsers which are based on the "WebKit" (Safari) triggers every three things: layout, painting, and composite which is bad for the performance.

The other thing is if we don't wait for loading the page our animations also aren't quite good. For example on this video titles are appearing not in a smooth way

gallery-safari-no-wait-for-load.mov

Performance on the gallery test page without waiting for the load

galler-no-wait-for-load

Here the green graph(frames per second) is very bumpy because animations start together with the other processes causes that the graph isn't smooth and frames per second aren't consistent

This is why I added a draft js script to test that.

Performance on the gallery test page waiting for the load(with the script).

gallery-wait-for-load

Here is the difference. Animation waits after the previous things are loaded and then triggers itself. Here frames per second are consistent and equal 60fps which is good.

So even if we want to animate things right after a page load (for example after internal linking to another page) I believe we should also wait till heavier processes are loaded and then trigger the CSS keyframes.

@mikemai2awesome Please take a look at this and let me know what do you think.

FYI @colbytcook

Copy link
Collaborator

@mikemai2awesome mikemai2awesome left a comment

Choose a reason for hiding this comment

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

@MarcinMr Good job! I only have a few inline comments and we'll discuss performance with the team in dev huddle.

@colbytcook colbytcook had a problem deploying to feature/DS-692-animation-tool-set--branch-preview January 27, 2022 12:44 Failure
Copy link
Collaborator

@mikemai2awesome mikemai2awesome left a comment

Choose a reason for hiding this comment

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

@MarcinMr I reduced cascade to 25 to keep the file size down. We will need to figure out how to do this better later on.

@colbytcook colbytcook temporarily deployed to feature/DS-692-animation-tool-set--branch-preview January 27, 2022 18:35 Inactive
@danielamorse danielamorse self-requested a review January 27, 2022 18:39
@colbytcook colbytcook had a problem deploying to feature/DS-692-animation-tool-set--branch-preview January 27, 2022 19:52 Failure
@colbytcook colbytcook requested a deployment to feature/DS-692-animation-tool-set--branch-preview January 27, 2022 19:57 Abandoned
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@MarcinMr I made a separate PR so you could review my suggestions. Feel free to merge that PR if it all makes sense: #2428

This all looks great, just had some notes on the cascade animation and the demo JS. See inline comments.

cc @mikemai2awesome @colbytcook

…on-tool-set--edits

DS-692 Animation Tool Set (edits)
@colbytcook colbytcook had a problem deploying to feature/DS-692-animation-tool-set--branch-preview February 25, 2022 15:50 Failure
@danielamorse danielamorse self-requested a review February 25, 2022 22:08
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

My JS edits have been merged and everything else looks great.

I just merged in the latest from master to fix the failing build (chromedriver).

@colbytcook colbytcook requested a deployment to feature/DS-692-animation-tool-set--c590233--commit-preview February 25, 2022 22:18 Abandoned
@danielamorse danielamorse merged commit a60400b into master Feb 25, 2022
@danielamorse danielamorse deleted the feature/DS-692-animation-tool-set branch February 25, 2022 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants