Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-gallery-carousel",
"version": "0.3.0",
"version": "0.4.0",
"description": "Mobile-friendly Carousel with batteries included (supporting touch, mouse emulation, lazy loading, thumbnails, fullscreen, RTL, keyboard navigation and customisations).",
"author": "yifaneye",
"license": "MIT",
Expand Down
1 change: 1 addition & 0 deletions src/components/Carousel/Carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ const GalleryCarousel = (props, ref) => {
widgetsHasShadow={props.widgetsHasShadow}
hasCaptions={settings.hasCaptions}
curIndex={slides.curIndex}
placeholderImg={props.placeholderImg}
/>
</div>
{widgets}
Expand Down
1 change: 1 addition & 0 deletions src/components/Carousel/props.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const propTypes = {
hasDotButtonsAtMax: largeWidgetPositions,
hasCaptionsAtMax: largeWidgetPositions,
hasThumbnailsAtMax: PropTypes.bool,
placeholderImg: PropTypes.string,
Copy link
Owner

Choose a reason for hiding this comment

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

I'd suggest renaming it to fallbackImg.

Copy link
Owner

Choose a reason for hiding this comment

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

And also in other props.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

leftIcon: PropTypes.node,
rightIcon: PropTypes.node,
playIcon: PropTypes.node,
Expand Down
40 changes: 35 additions & 5 deletions src/components/Image/Image.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React, { Fragment, useRef, useState } from 'react';
import React, { Fragment, useEffect, useRef, useState } from 'react';
import styles from './Image.module.css';
import { PLACEHOLDER_IMAGE } from './constants';
import { Caption } from '../Widgets';
import useIntersectionObserver from '../../utils/useIntersectionObserver';
import PropTypes from 'prop-types';
import PropTypes, { string } from 'prop-types';
import {
objectFitStyles,
largeWidgetPositions,
Expand Down Expand Up @@ -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);
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

We can simply assign the fallback image in the handleError function.

Copy link
Author

Choose a reason for hiding this comment

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

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.

useEffect(() => {
fetch(props.placeholderImg).then(res => {
Copy link
Owner

Choose a reason for hiding this comment

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

Does this implementation support local file paths?

Copy link
Author

Choose a reason for hiding this comment

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

Tested it with a local file and it should support them.

setIsValidFallback(res.status === 200);
Copy link
Owner

Choose a reason for hiding this comment

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

Could it be "HTTP 304 Not Modified" in some cases?

Copy link
Author

Choose a reason for hiding this comment

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

I've changed it to check for res.ok instead now, which should address this issue.

setIsLoading(false);
}).catch((_reason) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Here, finally statement can be used to combine setIsLoading(false);.

Copy link
Author

Choose a reason for hiding this comment

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

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.

setIsLoading(false);
})
}, []);
Copy link
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

Done.


const handleError = () => {
setHasError(true);
};

let { src, srcset, alt, thumbnail, ...otherImageProps } = props.image;

src = isInViewport && !hasError ? src : PLACEHOLDER_IMAGE;
src = isInViewport && !hasError ? src : (!isLoading && isValidFallback ? props.placeholderImg : PLACEHOLDER_IMAGE);
srcset = isInViewport && !hasError ? srcset : null;
thumbnail = isInViewport && !hasError ? thumbnail : PLACEHOLDER_IMAGE;
thumbnail = isInViewport && !hasError ? thumbnail : (!isLoading && isValidFallback ? props.placeholderImg : PLACEHOLDER_IMAGE);

return (
<>
Expand Down Expand Up @@ -81,11 +93,28 @@ export const Image = (props) => {

const { src, alt, srcset, thumbnail, ...otherImageProps } = props.image;

// This block checks whether the user specified fallback image actually works or not
const [isLoading, setIsLoading] = useState(true);
const [isValidFallback, setIsValidFallback] = useState(false);
useEffect(() => {
fetch(props.placeholderImg).then(res => {
setIsValidFallback(res.status === 200);
setIsLoading(false);
}).catch((_reason) => {
setIsLoading(false);
})
}, []);
Copy link
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

Done.


const handleError = (event) => {
event.target.src = isValidFallback && !isLoading ? props.placeholderImg : PLACEHOLDER_IMAGE;
};

const image = props.shouldLazyLoad ? (
<LazyLoadedImage
slidesContainerRef={props.slidesContainerRef}
image={props.image}
style={style}
placeholderImg={props.placeholderImg}
/>
) : (
<img
Expand Down Expand Up @@ -120,5 +149,6 @@ Image.propTypes = {
shouldLazyLoad: PropTypes.bool.isRequired,
slidesContainerRef: elementRef.isRequired,
hasCaption: largeWidgetPositions.isRequired,
widgetsHasShadow: PropTypes.bool.isRequired
widgetsHasShadow: PropTypes.bool.isRequired,
placeholderImg: PropTypes.string,
};
4 changes: 3 additions & 1 deletion src/components/Slide/Slide.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const Slide = (props) => {
widgetsHasShadow={props.widgetsHasShadow}
hasCaption={props.hasCaption}
slidesContainerRef={props.slidesContainerRef}
placeholderImg={props.placeholderImg}
/>
) : (
<UserSlide slide={props.slide} />
Expand All @@ -44,5 +45,6 @@ Slide.propTypes = {
hasCaption: largeWidgetPositions.isRequired,
slidesContainerRef: elementRef.isRequired,
reference: elementRef,
isCurrent: PropTypes.bool.isRequired
isCurrent: PropTypes.bool.isRequired,
placeholderImg: PropTypes.string,
};
4 changes: 3 additions & 1 deletion src/components/Slides/Slides.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const Slides = (props) => {
widgetsHasShadow={props.widgetsHasShadow}
hasCaption={props.hasCaptions}
isCurrent={index === props.curIndex}
placeholderImg={props.placeholderImg}
/>
);
})}
Expand All @@ -58,5 +59,6 @@ Slides.propTypes = {
objectFit: objectFitStyles.isRequired,
widgetsHasShadow: PropTypes.bool.isRequired,
hasCaptions: largeWidgetPositions.isRequired,
curIndex: PropTypes.number
curIndex: PropTypes.number,
placeholderImg: PropTypes.string,
};
Loading