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

implementation of the onStartLoad handler for gatsby-image #6702

Merged
merged 13 commits into from
Dec 7, 2018

Conversation

kaoDev
Copy link

@kaoDev kaoDev commented Jul 24, 2018

quick implementation for the feature request from the issue #6659.
I think this should be enough to handle the scenario described by @zionis137

With this change the gatsby-image component accepts a new optional prop onStartLoad, it is a function called with no arguments and expecting no return value.

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 24, 2018

Deploy preview for using-postcss-sass failed.

Built with commit bd2058a17c0acc4ce73d993c2f8ac27dab04cf06

https://app.netlify.com/sites/using-postcss-sass/deploys/5b746e3ac6aed608d33e1f53

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 24, 2018

Deploy preview for using-drupal ready!

Built with commit bd2058a17c0acc4ce73d993c2f8ac27dab04cf06

https://deploy-preview-6702--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 24, 2018

Deploy preview for gatsbygram ready!

Built with commit bd2058a17c0acc4ce73d993c2f8ac27dab04cf06

https://deploy-preview-6702--gatsbygram.netlify.com

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 24, 2018

Deploy preview for gatsbyjs failed.

Built with commit bd2058a17c0acc4ce73d993c2f8ac27dab04cf06

https://app.netlify.com/sites/gatsbyjs/deploys/5b746e38c6aed608d33e1f47

@@ -168,9 +168,19 @@ class Image extends React.Component {
this.handleRef = this.handleRef.bind(this)
}

componentDidMount() {
if (this.state.isVisible && typeof this.props.onStartLoad === `function`) {
this.props.onStartLoad()
Copy link
Contributor

@pieh pieh Jul 25, 2018

Choose a reason for hiding this comment

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

just checking here - this won't cause firing this "event" twice if image is visible in initial viewarea? maybe this should also check if window.IntersectionObserver is available?

Copy link
Author

Choose a reason for hiding this comment

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

good point, I just pushed a change to fire only if the image was not visible before

@pieh
Copy link
Contributor

pieh commented Jul 25, 2018

Should it fire if image is already cached in browser?

@kaoDev
Copy link
Author

kaoDev commented Jul 25, 2018

hmm, I would say so, for the user it's no difference if the image is loaded from a server or from the cache. And as the user of this component the onStartLoad handler marks the moment when the actual image DOM element is inserted, which is in case of cache loading direct after mounting.

@ghost
Copy link

ghost commented Jul 25, 2018

Or the function could have an boolean argument wasCached. Then the user can decide what he wants to do.

@kaoDev
Copy link
Author

kaoDev commented Jul 25, 2018

the onStartLoad handler now gets called with an object holding the wasCached information, by implementing this I also realised that the information if the image was already seen was written at the wrong moment. Instead of updating the cache state in the onLoad handler the cache entry was set in the constructor, whenever another gatsby-image with the same src was already constructed.

@KyleAMathews
Copy link
Contributor

Deploy preview for using-styled-components failed.

Built with commit bd2058a17c0acc4ce73d993c2f8ac27dab04cf06

https://app.netlify.com/sites/using-styled-components/deploys/5b746e3bc6aed608d33e1f5f

@KyleAMathews
Copy link
Contributor

Deploy preview for using-contentful failed.

Built with commit bd2058a17c0acc4ce73d993c2f8ac27dab04cf06

https://app.netlify.com/sites/using-contentful/deploys/5b746e3bc6aed608d33e1f62

@kaoDev
Copy link
Author

kaoDev commented Aug 15, 2018

those errors seem to have the origin in some other unrelated code, I tried to merge a clean master state (a release tag), but there are still errors.
So @KyleAMathews can you please have a look on this or give a hint how this automatic tests/deployments can be fixed on this branch?

@kaoDev
Copy link
Author

kaoDev commented Sep 5, 2018

hey @pieh can you have another look, I just cleaned up the rebase mess I pushed a few days back and all checks are green 😄

@pieh pieh self-assigned this Sep 5, 2018
Kalle Ott and others added 3 commits September 25, 2018 21:28
* master: (870 commits)
  fix(graphql-skip-limit): declare `graphql` peer dependency (gatsbyjs#10305)
  fix(gatsby-plugin-offline): gracefully degrade if appshell isn't precached (gatsbyjs#10329)
  Service workers note (gatsbyjs#10276)
  fix(docs): link fixes, podcast addition (gatsbyjs#10332)
  feat(docs): Create clearer pathways in docs (gatsbyjs#9898)
  feat(www): Rename community section to creators (gatsbyjs#10312)
  docs(graphql-reference): clarify filtering using comma/and operator (gatsbyjs#10321)
  chore(release): Publish
  feat(gatsby-plugin-sass): Support Dart SASS (gatsbyjs#10159)
  fix(gatsby-source-drupal): use basic auth credentials to fetch remote files as well. (gatsbyjs#10302)
  fix(gatsby-source-filesystem): allow empty password for basic auth in createRemoteFileNode (gatsbyjs#10280)
  docs(gatsby-remark-prismjs): Use Gatsby V2 project structure (gatsbyjs#10059)
  chore: update link for react-gatsby-firebase-authentication (gatsbyjs#10314)
  fix(www): Awesome Gatsby sidebar link (gatsbyjs#10313)
  Add thijs koerselman to creators list (gatsbyjs#10303)
  chore(release): Publish
  fix(gatsby-plugin-emotion): allow for React.Fragment shorthand syntax (gatsbyjs#10291)
  feat(www): Update starter cards (gatsbyjs#10258)
  Update index.md (gatsbyjs#10307)
  Update index.md (gatsbyjs#10306)
  ...
Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @kaoDev 👍

@pieh pieh merged commit 1d25a95 into gatsbyjs:master Dec 7, 2018
@gatsbot
Copy link

gatsbot bot commented Dec 7, 2018

Holy buckets, @kaoDev — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

m-allanson added a commit that referenced this pull request Dec 8, 2018
* master: (1421 commits)
  feat(gatsby-image): add onStartLoad prop  (#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (#10350)
  fix(www): Starters.yaml housekeeping (#10354)
  docs: add ttag starter (#10352)
  docs: document branching (#9983)
  plugin checker initial commit (#7062)
  docs: new starter features is required (#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (#10202)
  Add Birra Napoli to site showcase (#10344)
  Add BetterDocs to site showcase (#10349)
  chore(release): Publish
  Add option to keep metadata in files processed by `gatsby-plugin-sharp` (#10210)
  fix(gatsby): [loki] sync db autosaves (#10212)
  Add Ad Hoc Homework to sites.yml (#10346)
  fix(graphql-skip-limit): declare `graphql` peer dependency (#10305)
  fix(gatsby-plugin-offline): gracefully degrade if appshell isn't precached (#10329)
  Service workers note (#10276)
  fix(docs): link fixes, podcast addition (#10332)
  feat(docs): Create clearer pathways in docs (#9898)
  feat(www): Rename community section to creators (#10312)
  ...
m-allanson added a commit to Bouncey/gatsby that referenced this pull request Dec 10, 2018
* master: (35 commits)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (gatsbyjs#10202)
  Add Birra Napoli to site showcase (gatsbyjs#10344)
  Add BetterDocs to site showcase (gatsbyjs#10349)
  chore(release): Publish
  ...
m-allanson added a commit to markza/gatsby that referenced this pull request Dec 11, 2018
* master: (1432 commits)
  chore(release): Publish
  Fix Starter Library URL and update copy (gatsbyjs#10368)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (gatsbyjs#10202)
  Add Birra Napoli to site showcase (gatsbyjs#10344)
  ...
m-allanson added a commit to alexkirsz/gatsby that referenced this pull request Dec 12, 2018
* master: (1425 commits)
  showcase: Add TMDb Gatsby site (gatsbyjs#10411)
  chore(www): bump offline plugin version (gatsbyjs#10409)
  chore(release): Publish
  Fix Starter Library URL and update copy (gatsbyjs#10368)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  ...
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
* implementation of the onStartLoad handler for gatsby-image

* update to typescript declarations of gatsby-image

* changed double quotes to backticks

* updated declaration file to better represent current state of gatsby-image

* make sure onStartLoad is only fired once

* fixed image cache information store
provide cache information in the onStartLoad handler

* updatede readme

* better name for cache setter

* lint fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants