Document variable substitutions in hidden fields in forms by mkhatib · Pull Request #6486 · ampproject/amphtml

mkhatib · 2016-12-05T23:04:44Z

rudygalfi · 2016-12-05T23:25:11Z

A couple comments, one about implementation (sorry).

cc @bpaduch to make sure we're accounting for form analytics as part of our overall analytics documentation (along with other specialized cases like amp-carousel, amp-access, etc).

mkhatib · 2016-12-05T23:33:14Z

@rudygalfi You might need to submit your review for me to see the comments.

ghost · 2016-12-06T14:06:33Z

extensions/amp-form/amp-form.md

+## Variable Substitutions
+__(<a href="https://www.ampproject.org/docs/reference/experimental.html">experimental</a>)__
+
+`amp-form` allows [variable substitutions](../../spec/amp-var-substitutions.md) for inside hidden inputs with `default-value` attribute. On each submission, `amp-form` finds all `input[type=hidden]` inside the form and apply variable substitutions to `default-value` attribute and fill the result in the input `value`.


Rewrite as follows:

amp-form allows variable substitutions for inputs that are hidden and that have the default-value attribute. On each form submission, amp-form finds all input[type=hidden] inside the form and applies variable substitutions to the default-value attribute and fills in the result for the input value.

ghost · 2016-12-06T14:09:08Z

extensions/amp-form/amp-form.md

  <tr>
    <td width="40%"><strong>Availability</strong></td>
-    <td>Stable<br>(<a href="#custom-validations">Custom Validation still experimental - See below</a>)</td>
+    <td>Stable<br>Experimental Features:


Rewrite as follows:
Stable with the following Experimental features:

ghost · 2016-12-06T14:13:38Z

extensions/amp-form/amp-form.md

+
+`amp-form` allows [variable substitutions](../../spec/amp-var-substitutions.md) for inside hidden inputs with `default-value` attribute. On each submission, `amp-form` finds all `input[type=hidden]` inside the form and apply variable substitutions to `default-value` attribute and fill the result in the input `value`.
+
+Here's an example of how inputs are before and after substitutions:


In this example, it shows using variable substitution for "Platform Variables" (variable substitutions doc lists Platform & Analytics vars). Does variable substitution in forms only apply to platform vars or can they substitute in any of the analytics vars? If there's a limitation, please document it.

This only exposes platform variables, I believe analytics variables are only exposed through analytics. Will update to reflect.

ghost · 2016-12-06T14:14:17Z

extensions/amp-form/amp-form.md

+</form>
+```
+
+Once the user tries to submit the form, AMP will try to resolve the variables and update the fields values. For XHR submissions, all variables are likely to be substituted and resolved. However, in non-XHR GET submissions, values that requires async-resolution might not be available and might not be resolved if it has not previously been resolved. `CLIENT_ID` for example would not resolve if it wasn't resolved and cached previously.


change "fields" to "fields'"

rudygalfi · 2016-12-05T23:22:40Z

extensions/amp-form/amp-form.md

+## Variable Substitutions
+__(<a href="https://www.ampproject.org/docs/reference/experimental.html">experimental</a>)__
+
+`amp-form` allows [variable substitutions](../../spec/amp-var-substitutions.md) for inside hidden inputs with `default-value` attribute. On each submission, `amp-form` finds all `input[type=hidden]` inside the form and apply variable substitutions to `default-value` attribute and fill the result in the input `value`.


Sorry, this comment should have been in the implementation change, but wanted to ask here. Would an attribute name like data-value-for-substitution be better here. There's two question to unpack:
(a) Is using data- attributes a better approach here. (I'm not sure where to use them and when not.)
(b) "default" suggests to me something that's defined (in fact, it's the value used if another one doesn't come along to replace it). Here, however, what's really being constructed is a tokenized string. I wonder if we can make the attribute name clearer.

I am happy to change this, default-value was a quick suggestion from @cramforce and I took it and ran with it 😄.

(a) Is using data- attributes a better approach here. (I'm not sure where to use them and when not.)

Me and you both. I am never sure whether to choose to go with data- or not. I'd love for us to decide on a better way to choose and set of rules to follow when evaluating attributes naming.

(b) "default" suggests to me something that's defined (in fact, it's the value used if another one doesn't come along to replace it). Here, however, what's really being constructed is a tokenized string. I wonder if we can make the attribute name clearer.

We could rename default-value to resolve-value or something clearer, value-for-substitution also sounds weird but might be clearer.

I think I changed my mind. Lets just use value instead of default-value. Could this be implemented in terms of a modified urlReplacements…maybeExpandLink? And then use the same data- attribute to opt into replacement

Yeah sure, would this look like the following:

<input value="can: CANONICAL_URL, rand: RANDOM" data-amp-replace="CANONICAL_URL RANDOM">

amp-form would then call a modified maybeExpandLink with the expected attribute to replace (value in this case and href in links case).

@rudygalfi @cramforce does this sound better?

LGTM. Should data-amp-replace be a comma separated list?

I think we should change it to be space separated because that is more idiomatic on the web.

Sent out #6621 to update the format we're doing here. PTAL there and I'll update docs PR after we get it in.

rudygalfi · 2016-12-05T23:23:24Z

extensions/amp-form/amp-form.md

+</form>
+```
+
+Once the user tries to submit the form, AMP will try to resolve the variables and update the fields values. For XHR submissions, all variables are likely to be substituted and resolved. However, in non-XHR GET submissions, values that requires async-resolution might not be available and might not be resolved if it has not previously been resolved. `CLIENT_ID` for example would not resolve if it wasn't resolved and cached previously.


AMP will try to resolve the variables and update the value attribute of all fields with the appropriate substitutions.

might not be available due to having not been resolved previously.

mkhatib · 2016-12-14T22:40:23Z

PTAL 👀

rudygalfi · 2016-12-16T19:58:16Z

extensions/amp-form/amp-form.md

+    <td>Stable with the following Experimental features:
+       <ul>
+          <li><a href="#custom-validations">Custom Validation</a></li>
+          <li><a href="#variable-substitution">Variable Substitutions</a></li>


missing s: variable-substitutions

rudygalfi · 2016-12-16T20:00:37Z

extensions/amp-form/amp-form.md

+
+`amp-form` allows [platform variable substitutions](../../spec/amp-var-substitutions.md) for inputs that are hidden and that have the `data-amp-replace` attribute. On each form submission, `amp-form` finds all `input[type=hidden][data-amp-replace]` inside the form and applies variable substitutions to its `value` attribute and replaces it with the result of the substitution.
+
+It's important to provide the variables you are using for each substitution on each input through a space-separated strings in `data-amp-replace`, see examples below. AMP will not replace variables that are not explicitly provided.


Can this be cleaned up to: You must provide the variables you are using for each substitution on each input by specifying a space-separated string of the variables used in data-amp-replace (see example below). AMP will not replace variables that are not explicitly specified.

rudygalfi · 2016-12-16T20:10:02Z

LGTM, thanks!

…#6486)

Document variable substitutions in hidden fields in forms

eaf9281

mkhatib added NEEDS REVIEW Related to: Documentation labels Dec 5, 2016

mkhatib assigned rudygalfi and muxin Dec 5, 2016

ghost reviewed Dec 6, 2016

View reviewed changes

rudygalfi suggested changes Dec 7, 2016

View reviewed changes

erwinmombay removed the NEEDS REVIEW label Dec 7, 2016

mkhatib mentioned this pull request Dec 13, 2016

Update form-vars-sub to use data-amp-replace and value attrs #6621

Merged

update docs

a45636e

ghost approved these changes Dec 15, 2016

View reviewed changes

rudygalfi reviewed Dec 16, 2016

View reviewed changes

updates

bba5bc3

rudygalfi approved these changes Dec 16, 2016

View reviewed changes

mkhatib merged commit 7d7b1cc into ampproject:master Dec 16, 2016

mkhatib deleted the form-var-sub-docs branch December 16, 2016 20:59

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Document variable substitutions in hidden fields in forms (ampproject…

12d6ffd

…#6486)

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Document variable substitutions in hidden fields in forms (ampproject…

c59879f

…#6486)

torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jan 3, 2017

Document variable substitutions in hidden fields in forms (ampproject…

88697d6

…#6486)


		`amp-form` allows [variable substitutions](../../spec/amp-var-substitutions.md) for inside hidden inputs with `default-value` attribute. On each submission, `amp-form` finds all `input[type=hidden]` inside the form and apply variable substitutions to `default-value` attribute and fill the result in the input `value`.

		Here's an example of how inputs are before and after substitutions:


		`amp-form` allows [platform variable substitutions](../../spec/amp-var-substitutions.md) for inputs that are hidden and that have the `data-amp-replace` attribute. On each form submission, `amp-form` finds all `input[type=hidden][data-amp-replace]` inside the form and applies variable substitutions to its `value` attribute and replaces it with the result of the substitution.

		It's important to provide the variables you are using for each substitution on each input through a space-separated strings in `data-amp-replace`, see examples below. AMP will not replace variables that are not explicitly provided.

Conversation

mkhatib commented Dec 5, 2016

Uh oh!

rudygalfi commented Dec 5, 2016

Uh oh!

mkhatib commented Dec 5, 2016

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!

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!

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!

mkhatib commented Dec 14, 2016

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!

rudygalfi commented Dec 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants