Expose a prop allowing users to specify a custom fallback image #88
Conversation
|
Hi @jingdid ! Thank you very much! I will review it today! |
yarn.lock
Outdated
There was a problem hiding this comment.
Would you like to tell me what is the reason for the changes here? Could you please keep the yarn lockfile unchanged?
There was a problem hiding this comment.
Apologies, I did not notice I had changed the yarn lockfile. I think this is just a consequence of my trying to get the demo to run locally. I don't think any of my changes should actually require updates to the lockfile - I will remove this when I next update this pull request.
src/components/Image/Image.js
Outdated
| const [isLoading, setIsLoading] = useState(true); | ||
| const [isValidFallback, setIsValidFallback] = useState(false); | ||
| useEffect(() => { | ||
| fetch(props.placeholderImg).then(res => { |
There was a problem hiding this comment.
Does this implementation support local file paths?
There was a problem hiding this comment.
Tested it with a local file and it should support them.
src/components/Carousel/props.js
Outdated
| @@ -48,6 +48,7 @@ export const propTypes = { | |||
| hasDotButtonsAtMax: largeWidgetPositions, | |||
| hasCaptionsAtMax: largeWidgetPositions, | |||
| hasThumbnailsAtMax: PropTypes.bool, | |||
| placeholderImg: PropTypes.string, | |||
There was a problem hiding this comment.
I'd suggest renaming it to fallbackImg.
src/components/Image/Image.js
Outdated
| fetch(props.placeholderImg).then(res => { | ||
| setIsValidFallback(res.status === 200); | ||
| setIsLoading(false); | ||
| }).catch((_reason) => { |
There was a problem hiding this comment.
Here, finally statement can be used to combine setIsLoading(false);.
There was a problem hiding this comment.
This block has now been moved out of this file, and is used in such a way now that finally should no longer be needed.
src/components/Image/Image.js
Outdated
| }).catch((_reason) => { | ||
| setIsLoading(false); | ||
| }) | ||
| }, []); |
There was a problem hiding this comment.
Here is an error message:
ESLint: React Hook useEffect has a missing dependency: 'props.placeholderImg'. Either include it or remove the dependency array.(react-hooks/exhaustive-deps)
src/components/Image/Image.js
Outdated
| const [isValidFallback, setIsValidFallback] = useState(false); | ||
| useEffect(() => { | ||
| fetch(props.placeholderImg).then(res => { | ||
| setIsValidFallback(res.status === 200); |
There was a problem hiding this comment.
Could it be "HTTP 304 Not Modified" in some cases?
There was a problem hiding this comment.
I've changed it to check for res.ok instead now, which should address this issue.
src/components/Image/Image.js
Outdated
| }).catch((_reason) => { | ||
| setIsLoading(false); | ||
| }) | ||
| }, []); |
There was a problem hiding this comment.
Here is an error message:
ESLint: React Hook useEffect has a missing dependency: 'props.placeholderImg'. Either include it or remove the dependency array.(react-hooks/exhaustive-deps)
src/components/Image/Image.js
Outdated
| @@ -33,15 +33,27 @@ const LazyLoadedImage = (props) => { | |||
| // the low quality image (props.image.thumbnail) will be hidden | |||
| }; | |||
|
|
|||
| // This block checks whether the user specified fallback image actually works or not | |||
| const [isLoading, setIsLoading] = useState(true); | |||
| const [isValidFallback, setIsValidFallback] = useState(false); | |||
There was a problem hiding this comment.
What's the reason for handling the error of the fallback image? The fallback image is likely to be a static image, which is quite stable. The user can handle it using their code if they want to, isn't it?
There was a problem hiding this comment.
We can simply assign the fallback image in the handleError function.
There was a problem hiding this comment.
This was intended to catch cases where a user provides an invalid/broken url for the fallback image - in that event, I wanted to make sure that the carousel would end up falling back onto the built-in placeholder again.
- Refactor so that fallback image validity is only check once rather than making a request for every image in the carousel - Allow thumbnails to also use fallback image - Gracefully use default placeholder if fallback image prop is not specified
|
@yifaneye Welcome back! Apologies for the bother, but any chance you could re-review this pull request? I've since addressed your comments while also improving the changed behavior. |
This change allows a user to specify a custom fallback image by setting a
placeholderImgprop to a url like so:<Carousel images={images} placeholderImg="https://placekitten.com/400/230" />If the placeholder image url provided is invalid, the repository's default placeholder image is used instead.
Addresses issue #86