🐛Expose amp-form response from SSR in submit-success and submit-error events.
#25242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alanorozco
reviewed
Oct 24, 2019
GoTcWang
reviewed
Oct 24, 2019
dreamofabear
approved these changes
Oct 24, 2019
|
Won't approve since it's not needed from me. LGTM pending @choumx's last comment. |
| @@ -924,15 +924,18 @@ export class AmpForm { | |||
| /** | |||
| * Transition the form to the submit success state. | |||
| * @param {!JsonObject} result | |||
| * @param {!JsonObject} eventData | |||
| * @param {?JsonObject=} opt_eventData | |||
There was a problem hiding this comment.
Nit: This should be !JsonObject= unless you want to allow passing null.
There was a problem hiding this comment.
We do want to pass null if that is the value for response.body
caroqliu
added a commit
that referenced
this pull request
Nov 11, 2019
caroqliu
added a commit
to caroqliu/amphtml
that referenced
this pull request
Nov 11, 2019
caroqliu
added a commit
that referenced
this pull request
Nov 14, 2019
* Intended changes from PR #25242 * Will comments * Revert removal of `tryResolve()` as it creates friction in unit tests
micajuine-ho
pushed a commit
to micajuine-ho/amphtml
that referenced
this pull request
Dec 27, 2019
…ror` events. (ampproject#25242) * Pass response.body * rename var * Restructure * eventData but make it opt
micajuine-ho
pushed a commit
to micajuine-ho/amphtml
that referenced
this pull request
Dec 27, 2019
…ubmit-error` events. (ampproject#25242)" (ampproject#25470) This reverts commit 48d5ffd.
micajuine-ho
pushed a commit
to micajuine-ho/amphtml
that referenced
this pull request
Dec 27, 2019
…5515) * Revert "🐛Expose amp-form response from SSR in `submit-success` and `submit-error` events. (ampproject#25242)" This reverts commit 48d5ffd. * Add test case for bad json
micajuine-ho
pushed a commit
to micajuine-ho/amphtml
that referenced
this pull request
Dec 27, 2019
…t#25543) * Intended changes from PR ampproject#25242 * Will comments * Revert removal of `tryResolve()` as it creates friction in unit tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SSR responses get passed fully to the
submit-successandsubmit-errorevents on amp-form. These often include extra fields such as the HTML to render in the template. This change only exposes to the events the expected values of the amp-form response, such as data inserted into the form fields or relevant errors. b/138778080 cc/ @GoTcWang