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

feat(gatsby-plugin-image): object-fit polyfill for IE #29072

Merged
merged 5 commits into from
Jan 19, 2021

Conversation

ascorbic
Copy link
Contributor

Lazy-loads a polyfill for object-fit and object-position if the browser doesn't support them (so IE11).

[ch20513]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 18, 2021
@wardpeet
Copy link
Contributor

My main question would be, why not load it as part of gatsby-browser? It becomes part of the app bundle so we are sure that it's always loaded. We don't have to do any ref checks in the react components

@ascorbic
Copy link
Contributor Author

As in load it unconditionally? Right now it's only loading it for IE. If I load it conditionally in gatsby-browser, it'll still need the check before it's called in case it hasn't finished loading.

@wardpeet
Copy link
Contributor

You could put this in gatsby-browser.js. This will load object-fit only when it's necessary. It's a little bit easier to reason with than putting it inside component code.

export const onClientEntry = () => {
  // Object-fit/Object-position polyfill for gatsby-image (IE)
  const testImg = document.createElement(`img`)
  if (
    typeof testImg.style.objectFit === `undefined` ||
    typeof testImg.style.objectPosition === `undefined`
  ) {
    return import(
      /* webpackChunkName: "objectFitPolyfill" */ `objectFitPolyfill`
    ).then(mod => mod.default())
  }
}

@ascorbic
Copy link
Contributor Author

Right, but I still need to apply it inside the component, because otherwise it won't catch any images that aren't in the DOM at the point it loads. This is why I'm calling it on individual components rather than globally.

@wardpeet
Copy link
Contributor

wardpeet commented Jan 19, 2021

You're correct! We need to run on a single ref, I thought we had to run it once.

laurieontech
laurieontech previously approved these changes Jan 19, 2021
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Seems like it's fine to go forward with this version as both work and it's a matter of preference.

Co-authored-by: Ward Peeters <ward@coding-tech.com>
@ascorbic ascorbic added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: media Related to gatsby-plugin-image, or general image/media processing topics and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 19, 2021
@gatsbybot gatsbybot merged commit 3b4e8a5 into master Jan 19, 2021
@gatsbybot gatsbybot deleted the feat/ie-polyfill branch January 19, 2021 20:15
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
* wip

* Update polyfill

* Add webpackChunkName

Co-authored-by: Ward Peeters <ward@coding-tech.com>

* Format

Co-authored-by: Ward Peeters <ward@coding-tech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: media Related to gatsby-plugin-image, or general image/media processing topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants