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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃搱 Add last-page-visible trigger #22929

Merged
merged 2 commits into from Jun 19, 2019

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Jun 19, 2019

Closes #22901

@Enriqe Enriqe merged commit e50818e into ampproject:master Jun 19, 2019
@Enriqe Enriqe deleted the story-completed-trigger branch June 19, 2019 18:55
@gmajoulet
Copy link
Contributor

Quick question: can't we already do that with the story-page-visible trigger and the PAGE_INDEX + PAGE_COUNT variables?

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 19, 2019

I believe @jeffjose had an explicit request for it to be a trigger, since a variable would be more expensive for publishers. Is that correct Jeff?

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 19, 2019

Oh and also:

He wanted to support sending the story end to different request url. Let's still do different event types : ) Because event is one to one match to request.

You can read more of the discussion in #22901

@jeffjose
Copy link
Contributor

I believe @jeffjose had an explicit request for it to be a trigger, since a variable would be more expensive for publishers. Is that correct Jeff?

AMP Analytics is declarative in nature, with rudimentary support for some evaluations. We cannot do PAGE_INDEX + 1 = PAGE_COUNT.

Even if we did that, it'll be hooked into an enabled switch, which is evaluted on every event which is dropped everytime except the last page.

In short, a) cannot evaluate expressions b) and even if we did it, it'll be expensive.

@gmajoulet
Copy link
Contributor

Awesome, thanks for the context :)

thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* add last-page-visible trigger

* reword commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add a trigger for when a story is completed
5 participants