feat: add image fill prop - #1280
Conversation
Add a prop for component images to size the image to either "fill" or "fit".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
WalkthroughThe change adds the shared Sequence Diagram(s)sequenceDiagram
participant Editor
participant ImageWrapper
participant Image
participant Browser
Editor->>ImageWrapper: configure imageFillType
ImageWrapper->>Image: pass imageFillType
Image->>Image: map fit to contain or fill to cover
Image->>Browser: render image with computed object-fit
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
These components were written pre-heavy use of Codex and so we haven't really set a post-Codex testing strategy. I have mixed feelings on whether tests like this are useful -- more coverage is good, but also they tend to be a lot of test code for testing small functionality & are prone to constant changes. So, I'd tend to say that we shouldn't commit tests like this and keep relying on the screenshot tests, but I'm open to other opinions
There was a problem hiding this comment.
Agreed with this
For the other components we generally don't have tests like this
There was a problem hiding this comment.
Just a note bc this is fairly common - both your PR and my PR has a new migration, so whoever merges second will need to merge main and renumber the migration
| }; | ||
| } | ||
|
|
||
| const photoGalleryImageStylingFields = { |
There was a problem hiding this comment.
nit - just put this object inline on 117
| import { ImageType } from "@yext/pages-components"; | ||
| import { TranslatableString } from "./types.ts"; | ||
|
|
||
| export type ImageFillType = "fill" | "fit"; |
| import { removeMapboxApiKeyPropsMigration } from "../components/migrations/0078_remove_mapbox_api_key_props.ts"; | ||
| import { imageFillTypeMigration } from "../components/migrations/0079_image_fill_type.ts"; | ||
| import { ProductCard } from "../components/pageSections/ProductSection/ProductCard.tsx"; | ||
| import { ImageWrapper } from "../components/contentBlocks/image/Image.tsx"; |
There was a problem hiding this comment.
Don't import the components here -- if there's future changes, they will drift. I don't think migrate actually needs the components for this sort of test
There was a problem hiding this comment.
Agreed with this
For the other components we generally don't have tests like this
| @@ -0,0 +1,24 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
I don't think this test is super necessary
Can remove it
| }, | ||
| styles: { | ||
| aspectRatio: 1.78, | ||
| imageFillType: "fill" as const, |
There was a problem hiding this comment.
as const is kind of weird
3 participants
Add a prop for component images to size
the image to either "fill" or "fit".
Screen.Recording.2026-07-31.at.10.20.02.mov