-
Notifications
You must be signed in to change notification settings - Fork 267
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
base: main
Are you sure you want to change the base?
Conversation
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
Unknowns:
|
return ( | ||
<Image | ||
{...passthroughProps} | ||
{...mediaOptions?.image} | ||
data={data.image} | ||
focalPoint={focalPoint} |
There was a problem hiding this comment.
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?
WHY are these changes introduced?
Adds focal point support for
MediaImage
. Thanks for pairing @frandiox! 🙏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 underlyingImage
component.Following the liquid theme recommendations, if a focal point is provided we set an inline
object-fit
style andobject-position: {x,y}
. Both of these can be overwritten by the developer using thestyles
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.
Post-merge steps
Checklist