[amp-analytics] Added ability to use variables in selectors. #3281
Conversation
|
@avimehta Could you please explain why this is needed? |
|
Updated #3281 (comment) with a usecase. |
| // Expand the selector using variable expansion. | ||
| trigger['selector'] = this.expandTemplate_(trigger['selector'], | ||
| trigger); | ||
| return urlReplacementsFor(this.getWin()).expand(trigger['selector']) |
There was a problem hiding this comment.
One concern is that since uppercase CSS are allowed and the var names are pretty logical - we have a meaningful possibility of an accidental collision. WDYT? Is url replacements part necessary here?
There was a problem hiding this comment.
For the usecases I am aware of and the ones described in this issue, I think going with this.expandTemplate_ is enough. In that case, only the vars that publishers specify will get expanded and not the ones that platform specifies.
The reason to add urlReplacementsFor is of course: uniformity across all expansions.
One solution could be to remove urlReplacementsFor for now and add it back if someone wants it.
There was a problem hiding this comment.
One think it's a good first step. We'll just have to make a note in docs that only author-based vars are allowed here.
There was a problem hiding this comment.
Done. Can you take a look again? The test failure seems to be unrelated.
|
@dvoytenko ptal? The test failure is not related to this PR. |
|
@cramforce Can you take a look? I think @dvoytenko won't be able to get to this till late next week. Thanks |
|
Should this be documented anywhere? |
|
I was hoping to get this in prod without documentation and used by a select few before full documentation. Would that work? Opened #3809 |
|
LGTM |
This allows remote trigger config to specify a config like this:
Remote config:
```
{
on: 'click',
selector: '${likeButtonSelector}',
request: 'foo',
}
```
In page config:
```
{
vars: { likeButtonSelector: '#myLikeButton' }
}
```
|
It looks all good but for one small issue: it URI encodes the values and this is the wrong encoding for a CSS selectors. For example a valid selector like this: "img, div, button" (selecting either an image a div or a button) if used as a value of an AMP Var, becomes after expansion into "img%2Cdiv%2Cbutton" if the AMP var is used in the selector. Probably there are issues with other valid CSS selector characters as well. |
|
@Gueorgi Can you open a separate issue for what you mentioned? |
|
opened #4055 |
This allows remote trigger config to specify a config like this:
Remote config that could be provided by a vendor (like Google Tag Manager or something else). This allows GTM to provide a config that can work across various publishers.
In-page config provided by a publisher like nytimes.com. This way, they can use the remote config from GTM and give any id/class to their like button.
Overall, this will be used like this: