feat(ui-react-storage): add loadingElement to StorageImage #6448
Conversation
|
Previous PR |
| if (isLoading && loadingElement !== undefined) { | ||
| return loadingElement; | ||
| } |
There was a problem hiding this comment.
Leaning towards returning null if isLoading is true and no loadingElement is provided. What do you think @dbanksdesign
There was a problem hiding this comment.
I would like to add that if we plan to return null if isLoading is true, we do similar to what I had in the previous PR.
if (isLoading) {
return <View style={{ ...style }}></View>;
}
Returning null will cause an layout shift in user application, whereas if a user provides an style with fixed width and height, this will not cause an layout shift even if the loadingElement is not present.
There was a problem hiding this comment.
I think the PR is good as is. The customer could pass null to loadingElement and it would effectively work the way you are describing @calebpollman ?
There was a problem hiding this comment.
That's true @dbanksdesign. My concern here is that existing consumers will still have the issue that this PR is meant to address as there is no default loadingElement
|
docs/src/pages/[platform]/connected-components/storage/storageimage/props.ts
Show resolved
Hide resolved
| @@ -27,6 +27,27 @@ Storage Browser for Amazon S3 is an open source component that you can add to yo | |||
|
|
|||
| End users work with S3 _locations_ within the Storage Browser interface. _Locations_ are S3 buckets or prefixes that you authorize end users to access using Amazon S3 Access Grants or Identity and Access Management (IAM) policies, depending on your use case. When the Storage Browser component is first rendered, it will show the LocationsView which displays only the locations you have granted them access to. Once an end user has selected a location, they can browse the S3 bucket or prefix, and all the data contained further down the S3 resource path, but they cannot browse buckets or prefixes higher up the S3 resource path. | |||
|
|
|||
| ## Quick start | |||
There was a problem hiding this comment.
This diff should already be merged to main. Not sure why it eixsts here
| @@ -11,7 +11,7 @@ | |||
| "dependencies": { | |||
| "@aws-amplify/ui-react": "^6.1.0", | |||
| "react": "^18.3.0", | |||
| "next": "^14.2.21", | |||
| "next": "^14.2.25", | |||
There was a problem hiding this comment.
This diff is from other PR as well. Can you rebase your PR?
There was a problem hiding this comment.
Thank you, I will do them once this conversation is resolved
Description of changes
Currently, when using StorageImage, the user would see the fallback text while the image is still being fetched. The issue is more prominent when the network is throttled.
This PR adds an
loadingElementtoStorageImagePropsand uses the loading state exposed byuseGetUrland returns theloadingElementwhen loading, if provided.Issue #6254
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.