Skip to content
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

Add wrapperProps for <span> #70

Merged
merged 3 commits into from
May 29, 2020
Merged
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
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export default MyImage;
| useIntersectionObserver | `Boolean` | true | Whether to use browser's IntersectionObserver when available. |
| visibleByDefault | `Boolean` | false | Whether the image must be visible from the beginning. |
| wrapperClassName | `String` | | In some occasions (for example, when using a placeholderSrc) a wrapper span tag is rendered. This prop allows setting a class to that element. |
| wrapperProps | `Object` | null | Props that should be passed to the wrapper span |
| ... | | | Any other image attribute |


Expand Down Expand Up @@ -145,6 +146,7 @@ export default Article;
| threshold | `Number` | 100 | Threshold in pixels. So the component starts loading before it appears in the viewport. |
| useIntersectionObserver | `Boolean` | true | Whether to use browser's IntersectionObserver when available. |
| visibleByDefault | `Boolean` | false | Whether the component must be visible from the beginning. |
| wrapperProps | `Object` | null | Props that should be passed to the wrapper span |


## Using `trackWindowScroll` HOC to improve performance
Expand Down Expand Up @@ -193,6 +195,7 @@ You must set the prop `scrollPosition` to the lazy load components. This way, th
| placeholder | `ReactClass` | `<span>` | React element to use as a placeholder. |
| threshold | `Number` | 100 | Threshold in pixels. So the image starts loading before it appears in the viewport. |
| visibleByDefault | `Boolean` | false | Whether the image must be visible from the beginning. |
| wrapperProps | `Object` | null | Props that should be passed to the wrapper span |
| ... | | | Any other image attribute |

Component wrapped with `trackWindowScroll` (in the example, `Gallery`)
Expand Down
3 changes: 3 additions & 0 deletions src/components/LazyLoadComponent.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class LazyLoadComponent extends React.Component {
threshold,
useIntersectionObserver,
width,
wrapperProps,
} = this.props;

if (
Expand All @@ -82,6 +83,7 @@ class LazyLoadComponent extends React.Component {
threshold={threshold}
useIntersectionObserver={useIntersectionObserver}
width={width}
wrapperProps={wrapperProps}
/>
);
}
Expand All @@ -97,6 +99,7 @@ class LazyLoadComponent extends React.Component {
style={style}
threshold={threshold}
width={width}
wrapperProps={wrapperProps}
/>
);
}
Expand Down
6 changes: 6 additions & 0 deletions src/components/LazyLoadImage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class LazyLoadImage extends React.Component {
useIntersectionObserver,
visibleByDefault,
wrapperClassName,
wrapperProps,
...imgProps
} = this.props;

Expand All @@ -60,6 +61,7 @@ class LazyLoadImage extends React.Component {
useIntersectionObserver,
visibleByDefault,
width,
wrapperProps,
} = this.props;

return (
Expand All @@ -76,6 +78,7 @@ class LazyLoadImage extends React.Component {
useIntersectionObserver={useIntersectionObserver}
visibleByDefault={visibleByDefault}
width={width}
wrapperProps={wrapperProps}
>
{this.getImg()}
</LazyLoadComponent>
Expand All @@ -89,6 +92,7 @@ class LazyLoadImage extends React.Component {
placeholderSrc,
width,
wrapperClassName,
wrapperProps,
} = this.props;
const { loaded } = this.state;

Expand All @@ -114,6 +118,7 @@ class LazyLoadImage extends React.Component {
height: height,
width: width,
}}
{...wrapperProps}
>
{lazyLoadImage}
</span>
Expand Down Expand Up @@ -144,6 +149,7 @@ LazyLoadImage.propTypes = {
useIntersectionObserver: PropTypes.bool,
visibleByDefault: PropTypes.bool,
wrapperClassName: PropTypes.string,
wrapperProps: PropTypes.object,
};

LazyLoadImage.defaultProps = {
Expand Down
11 changes: 10 additions & 1 deletion src/components/PlaceholderWithoutTracking.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,14 @@ class PlaceholderWithoutTracking extends React.Component {
}

render() {
const { className, height, placeholder, style, width } = this.props;
const {
className,
height,
placeholder,
style,
width,
wrapperProps,
} = this.props;

if (placeholder && typeof placeholder.type !== 'function') {
return React.cloneElement(placeholder, {
Expand All @@ -136,6 +143,7 @@ class PlaceholderWithoutTracking extends React.Component {
className={className}
ref={el => (this.placeholder = el)}
style={styleProp}
{...wrapperProps}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think wrapperProps is needed here. This is the placeholder span, which is different from the wrapper span. The placeholder span is wrapped by the wrapper span and it gets replaced once the component or image are loaded. Hope it's not too confusing. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool - I will get this changed this weekend. Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Aljullu

Question about a bit of the code. What's the reasoning behind:

if ((!effect && !placeholderSrc) || visibleByDefault) {
  return lazyLoadImage;
}

It looks like all the other use cases get wrapped in the <span/> except for the ones that fall inside that conditional. I'm wondering if someone is using a placeholder and would expect it to also be wrapped in the span and have the wrapper props applied? If not, then I can just add a note in the documentation for that. If those elements should also wrap the placeholder span, then I can make that update too.

I just wanted to get your thoughts before making any code changes. ✌️

Copy link
Owner

Choose a reason for hiding this comment

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

It looks like all the other use cases get wrapped in the <span/> except for the ones that fall inside that conditional.

That's correct, the wrapper is only needed to display the placeholder image or when there is an effect. Otherwise, there is no need to render it.

I'm wondering if someone is using a placeholder and would expect it to also be wrapped in the span and have the wrapper props applied? If not, then I can just add a note in the documentation for that.

Yeah, we should make it clear the wrapper is only rendered when placeholderSrc or effect props are defined. Checking the docs, wrapperClassName already has an explanation for it, but we could expand it if it's not clear.

Hope that helps @johnstonbl01.

Copy link
Owner

Choose a reason for hiding this comment

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

Thinking about it twice, to reduce confusion it might make sense to rewrite the check you posted as:

const needsWrapper = (effect || placeholderSrc) && !visibleByDefault;
if (!needsWrapper && !wrapperClassName && !wrapperProps) {
	return lazyLoadImage;
}

So if wrapperClassName or wrapperProps are defined, the wrapper is rendered. I think that might be less confusing, thoughts?

Feel free to handle it in this PR or I can open a new one.

Copy link
Contributor Author

@johnstonbl01 johnstonbl01 May 26, 2020

Choose a reason for hiding this comment

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

That makes sense to me. I'll take care of it in this PR - thanks for the clarification! I should be able to get to this tomorrow. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - should be good to go now on these changes.

  • Removed wrapperProps from being passed to the placeholder
  • Updated the conditional to match your above suggestion (I agree it's easier to read)
  • Updated the docs for the wrapperProps to reflect that the wrapping span is rendered when using placeholderSrc and effect

If I missed anything, please let me know!

>
{placeholder}
</span>
Expand All @@ -155,6 +163,7 @@ PlaceholderWithoutTracking.propTypes = {
y: PropTypes.number.isRequired,
}),
width: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
wrapperProps: PropTypes.object,
};

PlaceholderWithoutTracking.defaultProps = {
Expand Down