Dynamically insert content into anchor href on click by rajkumarsrk · Pull Request #9086 · ampproject/amphtml

rajkumarsrk · 2017-05-02T04:03:39Z

Add support to dynamically add URL parameters to the link URL

Follows the same guidance of how the data-amp-replace works where the values are & separated
The dynamic URL parameters will be added only for URL's from

Same as that of origin URL
https
Whitelisted URL
data-amp-addparams has the same set of rules as that of data-amp-replace

Any value that is part of data-amp-addparams and that need to be replaced should have entry on data-amp-replace

If this changes looks good I will update the Readme.md files as well

Fixes #8594

jridgewell · 2017-05-02T18:36:40Z

src/service/url-replacements-impl.js

+   * @param {!Element} element An anchor element.
+   * @return {string} url
+   * @private
+   */


We can avoid this entirely by passing in a map as opt_bindings in the #expandSync call above.

As discussed with you on slack opt_bindings can be used for the replace use case only.

jridgewell · 2017-05-02T18:38:09Z

src/service/url-replacements-impl.js

+   * @return {string} url
+   * @private
+   */
+  extendURLParams_(url, element) {


We already have data-vars-* for <amp-analytics>, maybe use the same?

I can make it as data-vars-urlparam from data-amp-addparam but it wont align with naming of data-amp-replace. Is that fine ?

Also data-amp-addparam can have token values like c=CLIENT_ID(abc) which should be replaced by entries provided on data-amp-replace like

a.setAttribute('data-amp-replace', 'QUERY_PARAM CLIENT_ID'); a.setAttribute('data-amp-addparams', 'guid=123 c=CLIENT_ID(abc)');

In this if we have a different data attribute name like data-vars-* I guess it might create confusion . Please let me know your suggestion on this

Let's leave as is then.

jridgewell · 2017-05-02T20:50:10Z

src/service/url-replacements-impl.js

+   * @return {string} url
+   * @private
+   */
+  extendURLParams_(url, element) {


Let's leave as is then.

jridgewell · 2017-05-02T21:02:27Z

src/service/url-replacements-impl.js

+    const additionalURLParamaters = element.getAttribute('data-amp-addparams');
+    if (!additionalURLParamaters) {
+      return url;
+    }


We'll need this to use addParamsToUrl, but unfortunately we'll need to do some parsing.

// This should probably live in src/url.js function spaceDelimitedParamsToMap(params) { return parseQueryString(params.replace(/\s+/, '&')); } // Here, in this code return addParamsToUrl(url, spaceDelimitedParamsToMap(additionalURLParamaters));

Added spaceDelimitedParamsToMap to url.js and used addParamsToUrl. Now because of the url.js usage it got simplified

jridgewell

Code wise looks fine (minus the types), @rudygalfi would you like to review?

jridgewell · 2017-05-02T23:15:09Z

src/url.js

+/**
+ * Covert space delimited parameters to Map
+ * @param {string} url
+ * @return {string}


This return is incorrect. {!Object<string>}

My bad. Missed the JS doc. Checked in these 2 fixes

jridgewell · 2017-05-02T23:15:37Z

src/url.js

+
+/**
+ * Covert space delimited parameters to Map
+ * @param {string} url


This should params, not url

rajkumarsrk · 2017-05-02T23:36:30Z

Is this the correct place to update documentation
https://github.com/ampproject/amphtml/blob/b2d46035b609e4ee9fe569152385603a7a4b4ade/spec/amp-managing-user-state.md#task-5-using-client-id-in-linking-and-form-submission

jridgewell · 2017-05-02T23:39:47Z

And https://github.com/ampproject/amphtml/blob/d4bb91f952f8e52fe43ae4c1431933b4fdfde575/spec/amp-var-substitutions.md

rajkumarsrk · 2017-05-04T22:12:30Z

@jridgewell https://github.com/ampproject/amphtml/blob/d4bb91f952f8e52fe43ae4c1431933b4fdfde575/spec/amp-var-substitutions.md page has details about substitution. Should we have separate Readme.md file for the addition . Also please review the updated documentation at https://github.com/rajkumarsrk/amphtml/blob/b140953db8d289aab8aa8d14141d683a76332208/spec/amp-managing-user-state.md#task-5-using-client-id-in-linking-and-form-submission and let me know your feedback

jridgewell · 2017-05-04T22:20:28Z

These questions are better directed at @bpaduch. 😄

ghost · 2017-05-05T13:04:40Z

spec/amp-managing-user-state.md

 <a href="https://example.com/step2.html?ref_id=CLIENT_ID(uid)" data-amp-replace="CLIENT_ID">
 ```

+**Alternative solution for passing Client ID to the outgoing links:** Define the new query parameter `ref_id` as part of the data attribute `data-amp-addparams` and for queries that needs parameter substitution provide those details as part of `data-amp-replace`. With this approach the URL would look clean and the parameters specified on `data-amp-addparams` will be dynamically added


@rudygalfi - As you own this doc and are more familiar with the content, can you review that this is accurate and the correct location for it?

ghost · 2017-05-05T13:12:54Z

@rajkumarsrk

@rudygalfi owns the amp-managing-user-state doc and I'd like him to review it for accuracy. Some of it's above my head.
What would be the purpose of a separate readme? Can it not be just covered in https://github.com/ampproject/amphtml/blob/master/spec/amp-var-substitutions.md?

rajkumarsrk · 2017-05-05T20:35:12Z

I thought amp-var-substitutions.md was dedicated to substitution logic which has been shared with the analytics, anchour, form elements and the current change that we have in the PR is related to anchour and its on variable addition. If there is no concerns from your side I can go ahead and update it on the existing document itself (amp-var-substitutions.md)

rajkumarsrk · 2017-05-08T18:10:25Z

Updated the amp-var-substitutions.md as well

jridgewell · 2017-05-08T18:30:03Z

Looks like you have to rebase, then we can merge.

ghost · 2017-05-08T18:27:40Z

spec/amp-var-substitutions.md

 <a href="https://example.com?client_id=CLIENT_ID(bar)&abc=QUERY_PARAM(abc)" data-amp-replace="CLIENT_ID,QUERY_PARAM">Go to my site</a>
 ```

+Variable substitution works well with dynamic parameters added to link href through `data-amp-addparams`. Parameters provided as part of `data-amp-addparams` that requires variable substitution has to be specified on `data-amp-replace` like


I'd like to suggest you:

Add a level 4 subheading ( #### Appending parameters to the href).

Rewrite the information as follows (of course, assuming I'm technically correct ;) ):

If you need to append dynamic parameters to the href, specify the parameters by using the data-amp-addparams attribute. Any substitution parameters that you specify in data-amp-addparams must also be specified in data-amp-replace, as in the following example:

...

rajkumarsrk · 2017-05-08T18:55:37Z

@jridgewell The data-amp-replace is comma separated and currently I have it as data-amp-addparams space separated. And I am thinking of changing it to comma separating to make it consistent with data-amp-replace. Let me know your thoughts

jridgewell · 2017-05-08T19:05:10Z

Github says it still needs a rebase off current master.

And I am thinking of changing it to comma separating to make it consistent with data-amp-replace

Commas made sense for data-amp-replace because it's supposed to be a list. addparams is really a query string. If anything, I'd like to see us use &.

jridgewell · 2017-05-08T20:09:20Z

Ugh, unrelated docs failures. @erwinmombay, mind merging?

ghost

Doc change looks good.

…arams

…addclickparams

rajkumarsrk · 2017-05-10T18:04:16Z

After rebasing to the latest changes PR failure got fixed

jridgewell · 2017-05-10T18:12:47Z

Thanks for the help @rajkumarsrk!

rajkumarsrk · 2017-05-10T19:03:46Z

Thanks @jridgewell.

Can you let me know when we can expect this change on production ?

jridgewell · 2017-05-10T19:11:51Z

It'll be 2 weeks, though you'll be able to test using canary next week.

rajkumarsrk mentioned this pull request May 2, 2017

Dynamically insert content into anchor href on click #8594

Closed

jridgewell reviewed May 2, 2017

View reviewed changes

jridgewell requested changes May 2, 2017

View reviewed changes

googlebot added the cla: yes label May 2, 2017

jridgewell reviewed May 2, 2017

View reviewed changes

jridgewell approved these changes May 2, 2017

View reviewed changes

ghost reviewed May 5, 2017

View reviewed changes

ghost requested a review from rudygalfi May 5, 2017 13:13

rajkumarsrk force-pushed the addclickparams branch from bb0e735 to 19558fc Compare May 8, 2017 18:12

ghost suggested changes May 8, 2017

View reviewed changes

rajkumarsrk force-pushed the addclickparams branch from f3ebc14 to 54aeb48 Compare May 8, 2017 18:40

Adding dynamic URL params to the link URL

8dcddbd

rajkumarsrk force-pushed the addclickparams branch from c2eb374 to 8dcddbd Compare May 8, 2017 19:20

rajkumarsrk added 2 commits May 8, 2017 12:30

Adding dynamic URL params to the link URL

6cfbe68

Resolving merge conflicts

3fe5413

ghost approved these changes May 8, 2017

View reviewed changes

rajkumarsrk added 3 commits May 10, 2017 10:03

Adding dynamic URL params to the link URL

99656ff

Merge branch 'master' of github.com:ampproject/amphtml into addclickp…

02893e6

…arams

Merge branch 'addclickparams' of github.com:rajkumarsrk/amphtml into …

7b7d253

…addclickparams

jridgewell merged commit 21e3eb5 into ampproject:master May 10, 2017

ampprojectbot added the PR use: In Beta / Experimental label May 11, 2017

ampprojectbot added the PR use: In Stable label May 23, 2017

rajkumarsrk mentioned this pull request Jun 1, 2017

Option to add tracking parameters dynamically through data-amp-addparams for links on anchour tags #9673

Closed

Conversation

rajkumarsrk commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajkumarsrk commented May 2, 2017

Uh oh!

jridgewell commented May 2, 2017

Uh oh!

rajkumarsrk commented May 4, 2017

Uh oh!

jridgewell commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 5, 2017

Uh oh!

rajkumarsrk commented May 5, 2017

Uh oh!

rajkumarsrk commented May 8, 2017

Uh oh!

jridgewell commented May 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajkumarsrk commented May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented May 8, 2017

Uh oh!

jridgewell commented May 8, 2017

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

rajkumarsrk commented May 10, 2017

Uh oh!

jridgewell commented May 10, 2017

Uh oh!

rajkumarsrk commented May 10, 2017

Uh oh!

jridgewell commented May 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rajkumarsrk commented May 2, 2017 •

edited

Loading

rajkumarsrk commented May 8, 2017 •

edited

Loading