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

Fix passing onLoad overwrites, and call onLoad inside onImageLoad #120

Merged
merged 5 commits into from
Jun 3, 2023

Conversation

johnsonsu
Copy link
Contributor

Issue: Currently, there is no way to retrieve the onLoad event object with this library and passing an onLoad prop to LazyLoadImage will overwrite the handler on line 47 of LazyLoadImage.jsx causing afterLoad to never get called.

Description
This PR fixes passing onLoad to LazyLoadImage causing afterLoad() to never fire:

  • Fix onLoad getting overwritten by props which causes onImageLoad() to never get called.
  • preserve the functionality of onLoad by adding a callback.

- Fix onLoad getting overwritten by props which cause `onImageLoad()` to never get called.
- preserve the functionality of `onLoad` by adding a callback.
@Aljullu Aljullu added the enhancement New feature or request label May 24, 2023
@Aljullu Aljullu self-requested a review May 24, 2023 17:02
@Aljullu
Copy link
Owner

Aljullu commented May 24, 2023

Thanks for your contribution, @johnsonsu! I agree that it would be a good idea to pass the event object. However, it's not clear to me whether we need to introduce a new prop for this. Could we simply use afterLoad instead (maybe renaming it to onLoad)? Would that work for your use-case?

@johnsonsu
Copy link
Contributor Author

Hi @Aljullu, hope you are doing well! Yes, that will work for my use case. I can update my code to include the event in the afterLoad callback like:

return e => {
    this.props.afterLoad(e);
    ...

However, I think it's still a good idea to preserve the onLoad for consistency because your library carries over all other props of img and having onLoad as an exception is confusing. In fact, I created this PR because I was expecting onLoad to work just like a regular img but it didn't. What do you think?

@Aljullu
Copy link
Owner

Aljullu commented May 25, 2023

Good points, @johnsonsu. I'm thinking we should maybe simply rename the afterLoad to onLoad for the reasons you mention. And perhaps keep afterLoad as an alias of onLoad to keep backwards compatibility. Does it make sense to you? Are you up to giving it a spin? Ideally, we would update the docs as well.

@johnsonsu
Copy link
Contributor Author

Good points, @johnsonsu. I'm thinking we should maybe simply rename the afterLoad to onLoad for the reasons you mention. And perhaps keep afterLoad as an alias of onLoad to keep backwards compatibility. Does it make sense to you? Are you up to giving it a spin? Ideally, we would update the docs as well.

I think that make sense. I will update the code and docs and push it to this PR in a bit.

@johnsonsu
Copy link
Contributor Author

@Aljullu This is ready for your review.

Copy link
Owner

@Aljullu Aljullu left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks again for working on this. I left one comment inline with a suggestion. Also, you will need to add onLoad to the propTypes and defaultProps of that file.

Besides that, it might be a good idea to duplicate this test with the onLoad prop.

src/components/LazyLoadImage.jsx Outdated Show resolved Hide resolved
johnsonsu and others added 3 commits May 29, 2023 11:52
@johnsonsu
Copy link
Contributor Author

I think that's a good suggestion.

I've updated the code and labelled afterLoad as deprecated in the document and with comments.
I also updated the test to include both onLoad and afterLoad in the same test. This way the test can verify that passing onLoad no longer overwrites.

@johnsonsu johnsonsu requested a review from Aljullu May 30, 2023 21:51
Copy link
Owner

@Aljullu Aljullu left a comment

Choose a reason for hiding this comment

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

Awesome, this is looking good, @johnsonsu! Thanks a lot for your contribution! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants