-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
S3Upload UI fixes #15179
base: master
Are you sure you want to change the base?
S3Upload UI fixes #15179
Conversation
… S3Upload value in S3Upload settings
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
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.
LGTM but one small change needed to allow all normal bindings to work.
@@ -78,12 +83,15 @@ | |||
} | |||
|
|||
const upload = async () => { | |||
const processedFileKey = processStringSync(key, { | |||
[$component.id]: fieldState, |
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.
This means the only context available is itself, but we need to add the normal context in here too don't we? Since you have access to all normal bindings when configuring this setting in the builder.
So we just need to pull in normal context as well:
const context = getContext("context")
...
const processedFileKey = processStringSync(key, {
...$context,
[$component.id]: fieldState,
})
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.
Yep, that's correct, the other context elements should be present. $context
also contains the correct parameter so there's no need for the [$component.id]
entry now. Cheers!
… AWS put returns no body on a 200 so response.json would mistakenly throw an error
Description
A couple of minor fixes for the
S3Upload
component for issues that I came across while testing.Addresses
Fix
- When making aput
request to S3, the response body is empty. When the api client encountered this, the call toreponse.json()
was triggering an error. Instead a call to.text()
is made and verified. If the response text is empty, an empty object is returned instead.S3Upload
component in the builder the progress circle would appear and remain visibleS3Upload
picker value within its own settings. This is specifically useful if you want to map the name of a file you've added in file picker directly to theFile name
setting.{{ My File Upload.Value.0.name }}
{{ My File Upload.Value.0.type }}
Screenshots
Original S3 issue. This is now prevented
Launchcontrol
Minor fix for S3Upload component and added nested prop binding.