Skip to content

Fix sample for amp-form#5548

Closed
sebastianbenz wants to merge 2 commits intoampproject:masterfrom
sebastianbenz:patch-3
Closed

Fix sample for amp-form#5548
sebastianbenz wants to merge 2 commits intoampproject:masterfrom
sebastianbenz:patch-3

Conversation

@sebastianbenz
Copy link
Copy Markdown
Contributor

Post requests are only allowed via xhr.

Post requests are only allowed via xhr.
@jridgewell
Copy link
Copy Markdown
Contributor

/to @mkhatib

Copy link
Copy Markdown
Contributor

@mkhatib mkhatib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this, one change

```html
<form method="post" action="https://example.com/subscribe" target="_blank">
<form method="post"
action="https://example.com/subscribe"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please drop action from here - we don't allow action on post anymore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@sebastianbenz
Copy link
Copy Markdown
Contributor Author

Didn't realise action only works with GET. Updated the docs to reflect this as well.


Example:
```html
<form method="post" action="https://example.com/subscribe" target="_blank">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we still require target for all. Feel free to file a bug to remove that requirement from POST requests but please keep it in the examples for now.

<input type="submit" value="Subscribe">
</fieldset>
<div submit-success>
Subscription successful!
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably error, you'd need to encapsulate the text with a template element.


**target**
__required__
__required__ for `GET` requests.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: this is required for both get and post. Might not make sense for POST anymore feel free to file an issue so we can update the validator rules.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkhatib
Copy link
Copy Markdown
Contributor

mkhatib commented Oct 13, 2016

Approved with couple more changes.

@mkhatib
Copy link
Copy Markdown
Contributor

mkhatib commented Oct 18, 2016

@sebastianbenz did you manage to update this PR yet?

@mkhatib
Copy link
Copy Markdown
Contributor

mkhatib commented Oct 18, 2016

@sebastianbenz note that I sent a PR that touches on same path here: #5671

@mkhatib
Copy link
Copy Markdown
Contributor

mkhatib commented Nov 2, 2016

Closing this for #5986 - thanks @sebastianbenz

@mkhatib mkhatib closed this Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants