Skip to content

Relaxing some form requirements and errors#7530

Merged
mkhatib merged 3 commits intoampproject:masterfrom
mkhatib:target-action-omit
Feb 14, 2017
Merged

Relaxing some form requirements and errors#7530
mkhatib merged 3 commits intoampproject:masterfrom
mkhatib:target-action-omit

Conversation

@mkhatib
Copy link
Copy Markdown
Contributor

@mkhatib mkhatib commented Feb 14, 2017

Related #5607
Related #5670
Closes #5877

@mkhatib mkhatib requested a review from muxin February 14, 2017 02:24
'form target=%s is invalid can only be _blank or _top: %s', target, form);
if (actionXhr) {
checkCorsUrl(actionXhr);
if (!target) {
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 flip the if statement.

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

'form target=%s is invalid can only be _blank or _top: %s', target, form);
if (actionXhr) {
checkCorsUrl(actionXhr);
if (!target) {
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.

Add a test for no target case?

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.

expect it to be _top

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

@mkhatib
Copy link
Copy Markdown
Contributor Author

mkhatib commented Feb 14, 2017

PTAL 👀

@mkhatib mkhatib merged commit 1a5d893 into ampproject:master Feb 14, 2017
@mkhatib mkhatib deleted the target-action-omit branch February 14, 2017 21:17
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants