Skip to content

Comments

Nick's solution#5

Open
nickserv wants to merge 3 commits intomasterfrom
nicks-solution
Open

Nick's solution#5
nickserv wants to merge 3 commits intomasterfrom
nicks-solution

Conversation

@nickserv
Copy link
Member

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

@nicolasmccurdy looks good. does your validator prevent form submission?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, it doesn't. I'll fix that soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized that since the form method is #, this page shouldn't be making any actual network requests when you submit the form anyway. So if I'm understanding this correctly, the form already can't be submitted. Though I guess I could disable the button or do something similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed a commit that disables the button whenever anything is invalid.

Copy link
Member

Choose a reason for hiding this comment

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

Good stuff. The default behavior of form will attempt to post to the # action whatever it might be. Usually, you'll see use of .preventDefault here to prevent submission. The big concern here of course is reducing the number of calls to the server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, I forgot that you need to use that method for prevent form submission. Should I try that out, or is disabling the button good enough?

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.

2 participants