fix: classic hero image doesn't respect width prop #900
Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
WalkthroughThe HeroImage props gain an optional Sequence Diagram(s)sequenceDiagram
autonumber
participant Page
participant HeroSection
participant HeroImage
participant Image
Page->>HeroSection: render(props)
HeroSection->>HeroImage: render(imageProps, variant)
note right of HeroSection `#F0F4FF`: className changed — adds mx-auto\nand responsive max-width calc(...)
HeroImage->>HeroImage: evaluate variant
alt variant == "classic"
HeroImage->>Image: set width = styles.width
Note right of Image `#E8F7E7`: width applied (classic)
else variant != "classic"
HeroImage->>Image: set width = undefined
Note right of Image `#FFF3E0`: width omitted (compact/immersive/spotlight)
end
HeroImage->>Image: render(src, width, className...)
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/visual-editor/src/components/pageSections/HeroSection.tsx (1)
540-545: Classic image container: centering + width cap details
- Add block to ensure mx-auto reliably centers the image element.
- Provide a fallback for the CSS var to avoid unexpected behavior when the theme variable is absent.
- Behavior change: previously lg:max-w-none; now the image remains capped at large viewports. Confirm this is intended.
Suggested update:
- "mx-auto max-w-full sm:max-w-[calc(min(calc(100vw-1.5rem),var(--maxWidth-pageSection-contentWidth))-350px)] rounded-image-borderRadius" + "mx-auto block max-w-full sm:max-w-[calc(min(calc(100vw-1.5rem),var(--maxWidth-pageSection-contentWidth,1152px))-350px)] rounded-image-borderRadius"Please confirm the removal of lg:max-w-none is desired for classic. If not, we can append lg:max-w-none to preserve previous large-screen behavior. Also confirm --maxWidth-pageSection-contentWidth is guaranteed at runtime across themes.
packages/visual-editor/src/components/contentBlocks/image/HeroImage.tsx (2)
81-89: Width respected only for classic — update sizes to match layoutNow that classic uses a fixed width plus a responsive max-w cap, the current sizes hint may mislead the browser to pick larger sources than necessary. Align sizes with the container math for classic; keep existing hints for other variants.
- <Image + <Image image={resolvedImage} aspectRatio={styles.aspectRatio} - width={variant === "classic" ? styles.width : undefined} - className={className || "max-w-full rounded-image-borderRadius w-full"} - sizes={imgSizesHelper({ - base: "calc(100vw - 32px)", - md: "calc(maxWidth / 2)", - })} + width={variant === "classic" ? styles.width : undefined} + className={className || "max-w-full rounded-image-borderRadius w-full"} + sizes={ + variant === "classic" + ? imgSizesHelper({ + base: "calc(100vw - 32px)", + // Mirrors sm:max-w calc with a safe fallback for the CSS var. + sm: "calc(min(calc(100vw - 1.5rem), var(--maxWidth-pageSection-contentWidth, 1152px)) - 350px)", + }) + : imgSizesHelper({ + base: "calc(100vw - 32px)", + md: "calc(maxWidth / 2)", + }) + } />If imgSizesHelper doesn’t accept sm, pass a raw sizes string instead for the classic branch. This improves image selection and avoids over-downloading.
102-119: Aspect-ratio filtering by label is i18n-fragileFiltering aspect ratios by option.label can break under localization. Prefer filtering by a stable value key (e.g., option.value).
- fields.styles.objectFields.aspectRatio.options.filter( - (option: { label: string; value: string }) => - !["4:1", "3:1", "2:1", "9:16"].includes(option.label) - ) + fields.styles.objectFields.aspectRatio.options.filter( + (option: { value: string }) => + !["4:1", "3:1", "2:1", "9:16"].includes(option.value) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/visual-editor/src/components/contentBlocks/image/HeroImage.tsx(2 hunks)packages/visual-editor/src/components/pageSections/HeroSection.test.tsx(2 hunks)packages/visual-editor/src/components/pageSections/HeroSection.tsx(1 hunks)packages/visual-editor/src/vite-plugin/defaultLayoutData.ts(1 hunks)
🔇 Additional comments (2)
packages/visual-editor/src/components/contentBlocks/image/HeroImage.tsx (1)
29-29: Variant plumb-through verified and correctAll HeroImage usages properly receive variant: HeroSection passes it via
setDeepinto ImageSlot[0].props.variant (line 488–489), test fixtures include variant as "classic", and the migration sets variant with a fallback. No issues found.Note: The mechanism is
setDeep, notresolveData, but the plumbing is complete and working as intended.packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
1046-1046: Test fixtures correctly match runtime resolveData output; review contains contradictory guidance.The test fixtures at lines 1046 and 1205 correctly reflect what
resolveDatasets at runtime (HeroSection.tsx line 542–544). Both currently use identical className strings withoutblock. Addingblockonly to test would create divergence from production code, contradicting the review's own stated goal to "align test fixture with runtime logic."If
blockis truly needed for CSS robustness, it must be added to the runtime code first (HeroSection.tsx line 542–544), and the test would then automatically reflect that change. Do not modify test fixtures independently from their runtime counterparts.Likely an incorrect or invalid review comment.
...s/testing/screenshots/HeroSection/[mobile] [classic] version 30 props with entity values.png
Show resolved
Hide resolved
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
...enshots/HeroSection/[tablet] version 16 props using entity values with old CTA structure.png
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
1046-1046: Complex calc expression with hard-coded values.The className contains a nested calc expression with hard-coded values (350px and 1.5rem). While this appears to correctly implement responsive width constraints, the magic numbers make the calculation difficult to understand and maintain.
Consider whether these hard-coded values should be:
- Documented with inline comments explaining their purpose (e.g., what the 350px represents in the layout)
- Extracted to CSS variables or constants in the component implementation for better maintainability
Since this is test code reflecting the expected component output, the actual refactoring would need to occur in the component itself (HeroSection.tsx or related layout files).
Also applies to: 1205-1205
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [classic] version 30 props with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] default props with data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] default props with no data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] version 16 props using entity values with old CTA structure.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] version 16 props with missing ctaType.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] version 16 props with old CTA structure.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] version 9 props using constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (3)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx(2 hunks)packages/visual-editor/src/components/pageSections/HeroSection.tsx(1 hunks)packages/visual-editor/src/vite-plugin/defaultLayoutData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/visual-editor/src/components/pageSections/HeroSection.tsx
- packages/visual-editor/src/vite-plugin/defaultLayoutData.ts
🔇 Additional comments (1)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
1043-1047: LGTM! Test updates correctly reflect the width prop fix.The test updates properly reflect the fix for the classic hero image to respect the width prop:
- Line 1043 uses width: 100, matching the test document image dimensions
- Line 1202 uses width: 640, matching its constant value image dimensions
- The new className adds
mx-autofor horizontal centering and implements responsive max-width constraints- Changes are consistently applied to both classic variant test cases
Also applies to: 1202-1206
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx(2 hunks)packages/visual-editor/src/components/pageSections/HeroSection.tsx(2 hunks)packages/visual-editor/src/vite-plugin/defaultLayoutData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/vite-plugin/defaultLayoutData.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (2)
packages/visual-editor/src/components/pageSections/HeroSection.tsx (1)
421-421: Verify the default width value of 490px.The width has been changed from 640 to 490, but the rationale for this specific value isn't clear. This value should align with:
- The responsive max-width constraints in the className (especially
md:max-w-[350px])- The available space calculation at large viewports (
calc(...) - 350px)- Common image aspect ratios when combined with the default
aspectRatio: 1.78With
aspectRatio: 1.78andwidth: 490, the resulting height would be ~275px. Confirm this produces the intended visual result across the responsive breakpoints.packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
1043-1047: Confirm width/max-width constraint interaction is intentional.The verification confirms that the
widthprop is applied only whenvariant === "classic"(HeroImage.tsx line 84). Both test cases usevariant: "classic", so the width values will be rendered.However, the responsive max-width constraints in the className still create a sizing conflict:
- Test with
width: 100: less constrained, but only 100px vs 350px at md- Test with
width: 490: explicit conflict—image width prop is 490px butmd:max-w-[350px]constrains display to 350px at medium viewportsConfirm this is the intended responsive sizing behavior where the width prop establishes a base/intrinsic width while className provides responsive display constraints across breakpoints.
3 participants
Updates the classic hero image to respect the width prop, with a max width that corresponds to the viewport