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

Image focal point support #2139

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Image focal point support #2139

wants to merge 4 commits into from

Conversation

scottdixon
Copy link
Contributor

@scottdixon scottdixon commented May 22, 2024

WHY are these changes introduced?

Adds focal point support for MediaImage. Thanks for pairing @frandiox! 🙏

Screenshot 2024-05-23 at 10 10 54 AM

WHAT is this pull request doing?

Focal point x & y values are exposed in Media Presentation.asJson.

This PR updates the MediaFile component to detect focal point data for images and pass it through to the underlying Image component.

Following the liquid theme recommendations, if a focal point is provided we set an inline object-fit style and object-position: {x,y}. Both of these can be overwritten by the developer using the styles prop.

HOW to test your changes?

Set a focal point on an image (note: you'll need to do this in Admin > Content > Files) and then pass the MediaImage to the MediaFile component.

const {product, variants} = useLoaderData<typeof loader>();
const {media} = product;

return (
  <MediaFile
    data={media.nodes[0]}
    mediaOptions={{
      image: {
        aspectRatio: '1/1',
        sizes: '(min-width: 45em) 50vw, 100vw',
        width: 300,
        height: 300,
      },
    }}
  />
)
{
  product(handle: "some-product") {
    id
    title
    media(first: 1) {
      nodes {
        ... on MediaImage {
          __typename
          image {
            __typename
            id
            url
            width
            height
            altText
          }
        }
        presentation {
          asJson(format: IMAGE)
        }
      }
    }
  }
}

Post-merge steps

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

Copy link
Contributor

shopify bot commented May 22, 2024

Oxygen deployed a preview of your sd-media-focal-point branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
vite ✅ Successful (Logs) Preview deployment Inspect deployment May 23, 2024 4:48 AM
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment May 23, 2024 4:48 AM
subscriptions ✅ Successful (Logs) Preview deployment Inspect deployment May 23, 2024 4:48 AM
custom-cart-method ✅ Successful (Logs) Preview deployment Inspect deployment May 23, 2024 4:48 AM
optimistic-cart-ui ✅ Successful (Logs) Preview deployment Inspect deployment May 23, 2024 4:48 AM
Skeleton (skeleton.hydrogen.shop) ✅ Successful (Logs) Preview deployment Inspect deployment May 23, 2024 4:48 AM

Learn more about Hydrogen's GitHub integration.

@frandiox
Copy link
Contributor

Unknowns:

  • Will we be able to query presentation.asJson.focalPoint for images in variants? Or only for MediaImage?
  • If not, should we simplify the props received by the <Image/> component? Right now it supports presentation but we might want to change it to focalPoint directly if the SFAPI is not going to return presentation for normal images.
  • Should we start querying MediaImage instead of normal images in the templates by default to get access to presentation in the query?

return (
<Image
{...passthroughProps}
{...mediaOptions?.image}
data={data.image}
focalPoint={focalPoint}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Image objects don't contain presentation data (only MediaImage objects do). So instead of passing the entire presentation object through to <Image />, we've decided to make a focalPoint prop and just pass through the focal point json (e.g. { x: "0.5", y: "0.5"}) - as a bonus this allows developers to more easily pass a manual focal point. Any concerns here @benjaminsehl / does this make sense to you?

@wizardlyhel
Copy link
Contributor

wizardlyhel commented May 27, 2024

Will we be able to query presentation.asJson.focalPoint for images in variants? Or only for MediaImage?

From the looks of it, there is a limited SFAPI surface where MediaImage is implemented and product variant is not one of them.

If not, should we simplify the props received by the component? Right now it supports presentation but we
might want to change it to focalPoint directly if the SFAPI is not going to return presentation for normal images.

I can't seem to figure out what else could appear in the presentation json for image type. If we know for sure what presentation for media image will return, then having us parsing the focal point from presentation is still the better option since I don't think developers should ever manually set a focal point from the dev side

Should we start querying MediaImage instead of normal images in the templates by default to get access to presentation in the query?

I'm not sure either. the difference between images and medias connection is that images connection is only images while the medias connection is everything ... I think it really depends on the complexity need of the merchant for their UI? (ie, always put youtube vid link in page or extra the modal3D media to an icon to launch 3D modal viewer ... etc)

@blittle blittle mentioned this pull request May 28, 2024
@wizardlyhel wizardlyhel added the Blocked Progress on this issue is blocked by something outside of our control label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Progress on this issue is blocked by something outside of our control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants