-
Notifications
You must be signed in to change notification settings - Fork 2k
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
form: exclude own metadata, append result instead of overwriting #1686
Conversation
The 1st thing LGTM 👍 . We could land it first and publish as a patch, then nobody has to worry about upgrading. |
I could also add the second thing as an option, |
i could live with that :P |
…ult, with option to append
@goto-bus-stop updated, please take a look 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation LGTM
packages/@uppy/form/src/index.js
Outdated
@@ -23,6 +23,7 @@ module.exports = class Form extends Plugin { | |||
resultName: 'uppyResult', | |||
getMetaFromForm: true, | |||
addResultToForm: true, | |||
replaceResultInFormWithNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚲 🏚️ , but maybe the default option should be multipleResults: false
or combineMultipleResults: false
? i feel like replaceResultInFormWithNew
is very verbose but also doesn't immediately clarify what it's for.
3d63a2c
to
42032aa
Compare
42032aa
to
f68059e
Compare
This PR does 2 things:
Form
appended to a<form>
element, when gathering metadata from that form. Addresses ASSEMBLY_SATURATED when re-picking a file for robodog's form integration #1637, where because of this issue we were sending a huge amount of metadata totusd
— the wholecomplete
object, instead of metadata intended by the user/dev.value
sent to the server was an object, now it’s an array.