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

Add amp-fx="fade-in" - scroll triggered timed animation. #14351

Merged
merged 39 commits into from
Apr 5, 2018

Conversation

nainar
Copy link
Contributor

@nainar nainar commented Apr 2, 2018

This PR fixes #14351 adds the following functionality:

  • adds the ability to accept amp-fx="fade-in" which triggers a timed fade in animation
  • customizing the fade in animation by specifying:
    • duration of animation
    • easing curve of the animation
    • margin at which the animation should be triggered

Copy link
Contributor Author

@nainar nainar left a comment

Choose a reason for hiding this comment

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

@alanorozco could you PTAL?

I have highlighted the sections that need review. Since #14276 hasn't landed yet the diff has changes made in this PR and that one. Will rebase once #14276 has landed.

@@ -0,0 +1,112 @@
<!doctype html>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

@@ -134,6 +128,10 @@ class AmpFxCollection {

// Validate that we support the requested fx types.
fxTypes.forEach(fxType => {
if (fxType == FxType.FADE_IN) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved to the preset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,47 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

}
},
},
'fade-in': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

import {Services} from '../../../../src/services';
import {dev, user} from '../../../../src/log';
import {convertEasingKeyword} from './amp-fx-presets-utils';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

@@ -33,5 +33,10 @@
<div>
<h1 amp-fx="parallax"></h1>
</div>
<div amp-fx="fade-in"></div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

@@ -34,5 +34,10 @@ PASS
| <div>
| <h1 amp-fx="parallax"></h1>
| </div>
| <div amp-fx="fade-in"></div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

@@ -79,6 +79,38 @@ In this example, as the user scrolls the page, the h1 element scrolls faster rel
</h1>
```

### fade-in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks for updating the docs in the same PR!

I would add a note about fade-in being experimental.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -288,6 +288,12 @@ const EXPERIMENTS = [
spec: 'https://github.com/ampproject/amphtml/issues/9074',
cleanupissue: 'https://github.com/ampproject/amphtml/issues/14263',
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

@@ -4524,7 +4524,7 @@ attr_lists: {
# https://www.ampproject.org/docs/reference/components/amp-fx-collection
attrs: {
name: "amp-fx"
value_casei: "parallax"
value_regex_casei: "(fade-in|parallax)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New compared to #14276

@nainar nainar requested a review from alanorozco April 2, 2018 16:28
@nainar
Copy link
Contributor Author

nainar commented Apr 2, 2018

@alanorozco I have rebased on to the rewrite in #14276 so this should be much easier to read now.

}

// If above the threshold of trigger-position
if (!fxElement.isMutateScheduled()) {
Copy link
Member

Choose a reason for hiding this comment

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

  1. Short-circuit here.
  2. Maybe changing getResources() to mutateElement directly can save you a couple of steps (setIsMutateScheduled(), getElement()).
  3. Arrow func.
if (fxElement.isMutateScheduled()) {
   return;
}
fxElement.mutate(() => {
  // ...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the short circuiting here. However FxElement doesn't extend BaseElement so I don't think the mutateElement will work here, will it?

@@ -110,6 +118,19 @@ export class FxElement {

/** @private {number} */
this.factor_ = parseFloat(element.getAttribute('data-parallax-factor'));

Copy link
Member

Choose a reason for hiding this comment

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

What worries me about this style is that the specific FX are being scattered in more than one place.

For generic properties, like easing or duration, data-duration (without the preset name) should work fine. If they are to be invalid for certain presets, we can add validation rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I share your concern here. Made the change. We already have one preset specific attribute data-parallax-factor but I am going to talk to Ali about renaming that. That is a discussion for another thread though.

@@ -79,6 +79,38 @@ In this example, as the user scrolls the page, the h1 element scrolls faster rel
</h1>
```

### fade-in
Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks for updating the docs in the same PR!

I would add a note about fade-in being experimental.

@@ -134,6 +128,10 @@ class AmpFxCollection {

// Validate that we support the requested fx types.
fxTypes.forEach(fxType => {
if (fxType == FxType.FADE_IN) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved to the preset?

Copy link
Contributor Author

@nainar nainar left a comment

Choose a reason for hiding this comment

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

Uploaded a new commit with the changes you asked for. PTAL?

@@ -79,6 +79,38 @@ In this example, as the user scrolls the page, the h1 element scrolls faster rel
</h1>
```

### fade-in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -110,6 +118,19 @@ export class FxElement {

/** @private {number} */
this.factor_ = parseFloat(element.getAttribute('data-parallax-factor'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I share your concern here. Made the change. We already have one preset specific attribute data-parallax-factor but I am going to talk to Ali about renaming that. That is a discussion for another thread though.

@@ -134,6 +128,10 @@ class AmpFxCollection {

// Validate that we support the requested fx types.
fxTypes.forEach(fxType => {
if (fxType == FxType.FADE_IN) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

// If above the threshold of trigger-position
if (!fxElement.isMutateScheduled()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the short circuiting here. However FxElement doesn't extend BaseElement so I don't think the mutateElement will work here, will it?

@cvializ cvializ merged commit e554769 into ampproject:master Apr 5, 2018
powdercloud added a commit to powdercloud/amphtml that referenced this pull request Apr 10, 2018
powdercloud added a commit that referenced this pull request Apr 10, 2018
* Revision bump for #14351

* Revision bump and whitespace correction for #14464
glevitzky pushed a commit that referenced this pull request Apr 27, 2018
* WIP

* Refactor parallax code to make it easier to add new presets

* Force Travis checks

* Travis fixes

* Private -> protected

* Add support for amp-fx='fade-in'. Write example and validator tests accordingly as well

* Demos post UX review

* Add 'real world' example that uses amp-fx='fade-in'

* Commit worthy test

* Add support for easing keywords

* Only access private members through accessors

* Only access private members through accessors

* Remove demo files

* Add accessor for resources.

* Fix linter errors

* Formatting FUNS

* Fix markdown

* Moar demos

* Gate amp-fx='fade-in' experiment

* Edits before push

* markdown changes

* Bring back demos

* remove US demos

* Respond to alanorozco@'s comments

* Fix build failures

* Fix missing support check

* Travis fixes

* Cleanups

* Remove references to fade-in

* Don't use private members outside class

* OOPS
glevitzky pushed a commit that referenced this pull request Apr 27, 2018
* Revision bump for #14351

* Revision bump and whitespace correction for #14464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants