Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the option to use FormData to encode the form elements #153

Merged
merged 4 commits into from
Apr 29, 2018

Conversation

BehindTheMath
Copy link
Collaborator

If the form's enctype attribute is set to multipart/form-data, use FormData to encode the form's elements.

Fixes #132.

If the form's enctype attribute is set to "multipart/form-data",
use FormData to encode the form's elements.
@BehindTheMath BehindTheMath added this to the 0.2.6 milestone Apr 26, 2018
options.requestOptions.formData = new FormData(el)
}
else {
options.requestOptions.requestParams = parseFormElements(el)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove requestParams: [] from line 19 now, as this makes that initialisation redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -51,6 +51,9 @@ module.exports = function(location, options, callback) {
break
}
}
else if (requestOptions.formData) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should either add var formData = requestOptions. formData || null on line 9 to match the corresponding requestParams variable we use in this module, and then replace all other instances of requestOptions.formData with formData or remove var requestParams = requestOptions.requestParams || null from line 8 and replace all instances of requestParams with requestOptions.requestParams.

@robinnorth
Copy link
Collaborator

I think you'll also need to update your TypeScript definitions as part of this, is that correct?

@BehindTheMath
Copy link
Collaborator Author

I think you'll also need to update your TypeScript definitions as part of this, is that correct?

Good point.

Copy link
Collaborator

@robinnorth robinnorth 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 actioning my feedback, LGTM 👍

@BehindTheMath
Copy link
Collaborator Author

It's great to have a second pair of eyes on it. There's always something that I miss. So thanks for that.

@BehindTheMath BehindTheMath merged commit e49d894 into master Apr 29, 2018
@BehindTheMath BehindTheMath deleted the feature/form-data branch April 29, 2018 19:05
@robinnorth
Copy link
Collaborator

Nice one on getting this feature in there, I think it's probably time we pushed out a new version now!

@BehindTheMath
Copy link
Collaborator Author

I'm going to start working on that.

@robinnorth
Copy link
Collaborator

Great, let me know if there's anything I can do to help.

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.

None yet

2 participants