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

<amp-analytics> not possible to be added inside of <amp-list> #3857

Closed
djamrozik opened this issue Jun 30, 2016 · 11 comments
Closed

<amp-analytics> not possible to be added inside of <amp-list> #3857

djamrozik opened this issue Jun 30, 2016 · 11 comments

Comments

@djamrozik
Copy link

Short description

Inside of the <template> tag of <amp-list>, I have items which I need to track using some of the features of <amp-analytics> (I'm adding this tag inside of the template). However, whenever added inside of this tag, the following errors occur and the template does not render at all.

Uncaught Error: No request strings defined. Analytics data will not be sent from this page. (log.js:335)
Uncaught Error: No triggers were found in the config. No analytics data will be sent.​​​ (log.js:335)

How do we reproduce the issue?

This can be reproduced by pasting a basic <amp-analytics> script inside of <amp-list>

Example:

Take the simple below <amp-analytics> script and paste inside of the given <amp-list> example

<amp-analytics>
    <script type="application/json">
    {
        "requests":{
            "isVisible":"/isVisible"
        },
        "triggers":{
            "isVisible":{ 
                "on":"visible",
                "request":"isVisible"
            }
        }
    }
    </script>
</amp-analytics>

<amp-list> examples (occurs in all) : https://ampbyexample.com/components/amp-list/

This affects all browsers, using latest version

@rudygalfi
Copy link
Contributor

"on": "visible" wouldn't work this way in any case since it's keyed on page visibility. By the time the amp-list is working, the page visible event has already passed. I suppose an exception to this would be the newly added visibilitySpec features that were added.

Can you elaborate on the use case? I'm curious what kind of setup requires bringing in the amp-analytics functionality within a list item and that cannot be incorporated in a statically declared amp-analytics config.

cc @avimehta

@djamrozik
Copy link
Author

@rudygalfi Sorry, I was just using that as a sample script in amp-analytics. (which works outside amp-list but produces that error inside)

An actual use case would be rendering a long list of items through amp-list and adding a visibility trigger based on the item's id (selector based visibility, added recently in #3512). I.e. when the item has been in viewport for enough time, fire the request (where the request URL would be provided inside of the new amp-analytics element created).

@src-code
Copy link
Contributor

src-code commented Jul 1, 2016

It seems like this is more of an issue for vars, which would be addressed by upcoming analytics support for vars in custom data attributes. @djamro2's viewability example could be addressed by putting a blanket trigger on each element of a certain type, and then the data attributes could be used to provide item-specific data for the beacon as each item becomes viewable.

@djamrozik
Copy link
Author

I believe the custom data attributes will work exactly for this use case.

On another point, I'm not sure if maybe the error logs or documentation should be updated about the case where amp-analytics is inside amp-list. If so, I'll be happy to make the pull request, but would need some direction on how to update this.

And if not, feel free to close this issue, thanks.

@adelinamart
Copy link
Contributor

Hi. Are there still any open questions here? Thanks.

@rudygalfi
Copy link
Contributor

I think this might still be blocked by #4449, but we've launched the ability to have element-level visibility and click-tracking, as well as data- params that can be put on elements to transport additional contextual data regarding the event: https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/analytics-vars.md#variables-as-data-attribute.

Closing this for now, but please reopen if there's some additional aspect here that isn't covered by the above.

@rudygalfi rudygalfi modified the milestones: New FRs, Pending Triage Feb 8, 2017
@grahamle
Copy link

grahamle commented Jan 8, 2018

Hi, @rudygalfi we just encountered the same issue when our business needs to analyze the product list data, while this kind of data in productList page is definitely async and rendered, thus we can't have this areas to be analized via <amp-analytics>. As we know, that's mainly because we can't nest our <amp-analytics> inside an <amp-list>. But that's quite important for us to have this feature, otherwise we will lost a lot.

For now, we've already tried with nesting <amp-pixel> inside the <amp-list>, but it didn't satisfy us either, because it can not trigger event accordingly.

And for sure, we've already tried data-vars-xxx, it doesn't work, because our data is totally async and when it came back and render out the DOM, I can't set up the <amp-analytics>.

Actually we hope we could do it like this way, as follows.

<amp-list>
    <div>
        <amp-img>
            <amp-analytics>
            </amp-analytics>
        </amp-img>
    </div>
    <div>
        <amp-img>
            <amp-analytics>
            </amp-analytics>
        </amp-img>
    </div>
</amp-list>

@zhouyx zhouyx reopened this Jan 8, 2018
@zhouyx
Copy link
Contributor

zhouyx commented Jan 8, 2018

I've looked deeper into this issue. It is NOT that <amp-analytics> can't be nested inside an <amp-list>. The reason here is that we don't allow <script> in mustache template.

@rudygalfi We should consider allowing script of type application/json in mustache template.

@grahamle One workaround for you now is to use remote config instead of inline script config. <amp-analytics config='remote_url'> Please let us know.

@rudygalfi
Copy link
Contributor

Let us know if @zhouyx's suggestion works for the short-term. Overall, I think we're interested to fix this, but we need to investigate how to do so.

@rudygalfi rudygalfi assigned zhouyx and unassigned rudygalfi Jan 8, 2018
@rudygalfi rudygalfi modified the milestones: New FRs, H1 January Jan 8, 2018
@zhouyx
Copy link
Contributor

zhouyx commented Jan 10, 2018

Realized we talked about allowing json script in
#11874 (comment)
#12023

@zhouyx
Copy link
Contributor

zhouyx commented Jan 18, 2018

Closing in favor of #12023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants