fix: fixed header rendering flash and product card image ratio #946
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. |
commit: |
WalkthroughThis PR removes the "sticky" headerPosition option and consolidates it as "fixed", replaces internal header-height measurement and spacer logic with a simplified wrapper, adds a migration to convert existing "sticky" values to "fixed", changes several DropZone/layout styles from Possibly related PRs
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: 2
🧹 Nitpick comments (4)
packages/visual-editor/src/components/header/SecondaryHeaderSlot.tsx (1)
120-130: VerifySlotactually appliesstyleto the element you intend (and consider Tailwindw-fit).This fix likely addresses the editor/live mismatch, but it relies on
@measured/puck’sSlotforwardingstyleto the correct DOM node (wrapper vs inner). If it doesn’t, consider wrapping the slot in a<div className="w-fit">…</div>or usingclassName="w-fit h-auto"directly on the slot (if supported) to stay consistent with the rest of the Tailwind usage.packages/visual-editor/src/docs/ai/components.d.ts (1)
523-528: Type narrowing looks correct; consider updating the docstring wording.
headerPosition: "sticky" | "scrollsWithPage"aligns with the component changes. The comment “Whether the header is ‘sticky’ or not” is slightly misleading now that this is a two-value mode; consider rewording to “Header positioning behavior”.packages/visual-editor/src/components/header/ExpandedHeader.tsx (1)
27-32: LGTM on removing “fixed”; drop the now-unnecessary fragment wrapper.
headerPosition: "sticky" | "scrollsWithPage"and the simplified wrapper are consistent with the new approach. Since you’re no longer conditionally rendering a spacer, the outer<>...</>can be removed.const ExpandedHeaderWrapper: PuckComponent<ExpandedHeaderProps> = ({ styles, slots, }) => { return ( - <> - <div className={headerWrapper({ position: styles.headerPosition })}> + <div className={headerWrapper({ position: styles.headerPosition })}> {/* Secondary Header (Top Bar) */} <div className="hidden md:flex"> <slots.SecondaryHeaderSlot style={{ height: "auto", width: "100%" }} /> </div> {/* Primary Header w/ nav bar */} <slots.PrimaryHeaderSlot style={{ height: "auto" }} /> - </div> - </> + </div> ); };Also applies to: 105-118
packages/visual-editor/src/components/migrations/0046_removed_fixed_header.ts (1)
1-13: Migration logic is correct; consider avoiding in-place mutation (optional).Current implementation works, but returning a copied object is a bit safer if
propsmight be reused elsewhere.export const removeFixedHeader: Migration = { ExpandedHeader: { action: "updated", propTransformation: (props) => { if (props.styles?.headerPosition === "fixed") { - props.styles.headerPosition = "sticky"; + return { + ...props, + styles: { ...props.styles, headerPosition: "sticky" }, + }; } - return props; + return props; }, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[desktop] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (12)
packages/visual-editor/src/components/configs/directoryConfig.tsx(1 hunks)packages/visual-editor/src/components/configs/locatorConfig.tsx(1 hunks)packages/visual-editor/src/components/configs/mainConfig.tsx(1 hunks)packages/visual-editor/src/components/header/ExpandedHeader.test.tsx(1 hunks)packages/visual-editor/src/components/header/ExpandedHeader.tsx(2 hunks)packages/visual-editor/src/components/header/SecondaryHeaderSlot.tsx(1 hunks)packages/visual-editor/src/components/migrations/0046_removed_fixed_header.ts(1 hunks)packages/visual-editor/src/components/migrations/migrationRegistry.ts(2 hunks)packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx(1 hunks)packages/visual-editor/src/docs/ai/components.d.ts(1 hunks)packages/visual-editor/src/docs/components.md(1 hunks)packages/visual-editor/src/vite-plugin/templates/locator.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T22:00:03.843Z
Learnt from: mkouzel-yext
Repo: yext/visual-editor PR: 833
File: packages/visual-editor/src/components/Locator.tsx:1050-1057
Timestamp: 2025-10-29T22:00:03.843Z
Learning: In packages/visual-editor/src/components/Locator.tsx, the AppliedFilters component is intentionally rendered in two locations (inside the filter modal and outside it) as per the design requirements. This dual rendering should not be flagged as a duplicate issue.
Applied to files:
packages/visual-editor/src/vite-plugin/templates/locator.tsx
🧬 Code graph analysis (1)
packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
packages/visual-editor/src/components/migrations/0046_removed_fixed_header.ts (1)
removeFixedHeader(3-13)
🔇 Additional comments (7)
packages/visual-editor/src/components/configs/directoryConfig.tsx (1)
65-69: LGTM! Enables sticky positioning while preserving layout.The change from
height: "100vh"tominHeight: "100vh"allows content to grow beyond the viewport while maintaining the minimum height needed to keep the footer at the bottom. This enables sticky header positioning as described in the PR objectives.packages/visual-editor/src/components/configs/mainConfig.tsx (1)
75-79: LGTM! Consistent change across configs.The change from
height: "100vh"tominHeight: "100vh"is consistent with the other config files and enables sticky header functionality while preserving footer placement.packages/visual-editor/src/components/configs/locatorConfig.tsx (1)
64-68: LGTM! Completes consistent styling across all configs.The change from
height: "100vh"tominHeight: "100vh"is consistent with the other config files (directoryConfig and mainConfig) and enables the sticky header fix described in the PR objectives.packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx (1)
339-343: LGTM! Rendering fix correctly applied.The inline style
height: "fit-content"combined with the responsive minimum height ensures consistent image rendering between the editor and live page, addressing the stated PR objective.packages/visual-editor/src/vite-plugin/templates/locator.tsx (2)
107-120: LGTM: Proper data transformation pattern.Moving the migration and resolution logic into
transformPropsis the correct approach. This ensures data is fully resolved before the component renders, eliminating the rendering flash mentioned in the PR objectives.
144-144: LGTM: Correctly uses pre-resolved data.Passing the pre-resolved
datafrom props (prepared bytransformProps) to theRendercomponent eliminates the need for inline migration during render. This architectural improvement directly addresses the rendering flash issue and follows React best practices by separating data transformation from rendering.packages/visual-editor/src/docs/components.md (1)
276-280: Docs update matches the newheaderPositioncontract.The props table correctly reflects
"sticky" | "scrollsWithPage".
packages/visual-editor/src/components/header/ExpandedHeader.test.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/migrations/migrationRegistry.ts
Outdated
Show resolved
Hide resolved
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
asanehisa
left a comment
asanehisa
left a comment
There was a problem hiding this comment.
nice! awesome we can get the header "fixed" fixed haha
mkilpatrick
changed the title
packages/visual-editor/src/components/header/ExpandedHeader.tsx
Outdated
Show resolved
Hide resolved
|
| Key | Languages Removed |
|---|---|
fields.options.sticky |
cs,da de,en en-GB,es et,fi fr,hr hu,it ja,lt lv,nb nl,pl pt,ro sk,sv tr,zh |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/docs/components.md (1)
276-279: Consider clarifying the headerPosition description.The description "Whether the header is fixed to the top of the page or not" reads like a boolean choice, but the property accepts two distinct values:
"fixed"(header remains at top) and"scrollsWithPage"(header scrolls normally). Consider revising to something like: "Controls header positioning: 'fixed' keeps it at the top, 'scrollsWithPage' allows it to scroll with content."</review_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
packages/visual-editor/locales/cs/visual-editor.json(0 hunks)packages/visual-editor/locales/da/visual-editor.json(0 hunks)packages/visual-editor/locales/de/visual-editor.json(0 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(0 hunks)packages/visual-editor/locales/en/visual-editor.json(0 hunks)packages/visual-editor/locales/es/visual-editor.json(0 hunks)packages/visual-editor/locales/et/visual-editor.json(0 hunks)packages/visual-editor/locales/fi/visual-editor.json(0 hunks)packages/visual-editor/locales/fr/visual-editor.json(0 hunks)packages/visual-editor/locales/hr/visual-editor.json(0 hunks)packages/visual-editor/locales/hu/visual-editor.json(0 hunks)packages/visual-editor/locales/it/visual-editor.json(0 hunks)packages/visual-editor/locales/ja/visual-editor.json(0 hunks)packages/visual-editor/locales/lt/visual-editor.json(0 hunks)packages/visual-editor/locales/lv/visual-editor.json(0 hunks)packages/visual-editor/locales/nb/visual-editor.json(0 hunks)packages/visual-editor/locales/nl/visual-editor.json(0 hunks)packages/visual-editor/locales/pl/visual-editor.json(0 hunks)packages/visual-editor/locales/pt/visual-editor.json(0 hunks)packages/visual-editor/locales/ro/visual-editor.json(0 hunks)packages/visual-editor/locales/sk/visual-editor.json(0 hunks)packages/visual-editor/locales/sv/visual-editor.json(0 hunks)packages/visual-editor/locales/tr/visual-editor.json(0 hunks)packages/visual-editor/locales/zh/visual-editor.json(0 hunks)packages/visual-editor/src/components/header/ExpandedHeader.tsx(3 hunks)packages/visual-editor/src/components/migrations/0046_merge_sticky_and_fixed_header.ts(1 hunks)packages/visual-editor/src/components/migrations/migrationRegistry.ts(2 hunks)packages/visual-editor/src/docs/ai/components.d.ts(1 hunks)packages/visual-editor/src/docs/components.md(1 hunks)
💤 Files with no reviewable changes (24)
- packages/visual-editor/locales/es/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/lt/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/locales/hr/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/nl/visual-editor.json
- packages/visual-editor/locales/en/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/da/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/fr/visual-editor.json
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/visual-editor/src/docs/ai/components.d.ts
- packages/visual-editor/src/components/header/ExpandedHeader.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T20:17:30.322Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 946
File: packages/visual-editor/src/components/migrations/migrationRegistry.ts:47-48
Timestamp: 2025-12-12T20:17:30.322Z
Learning: In the yext/visual-editor repository, empty object placeholders (`{}`) in the migrationRegistry array are sometimes intentionally used to reserve migration slots for parallel PRs to prevent merge conflicts and maintain consistent migration indices.
Applied to files:
packages/visual-editor/src/components/migrations/migrationRegistry.ts
🧬 Code graph analysis (1)
packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
packages/visual-editor/src/components/migrations/0046_merge_sticky_and_fixed_header.ts (1)
mergeStickyAndFixedHeader(3-13)
⏰ 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). (4)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: create-dev-release
- GitHub Check: semgrep/ci
🔇 Additional comments (3)
packages/visual-editor/src/components/migrations/migrationRegistry.ts (2)
47-47: LGTM!The import and registration of the new migration
mergeStickyAndFixedHeaderis correctly implemented. The migration will transform existing "sticky" headerPosition values to "fixed" as intended.</review_comment_end>
99-100: LGTM - placeholder is intentional.Based on learnings, the empty object placeholder at line 99 is intentionally reserved for migration 0045 from a parallel PR to prevent index conflicts.
</review_comment_end>
packages/visual-editor/src/components/migrations/0046_merge_sticky_and_fixed_header.ts (1)
3-13: LGTM!The migration correctly transforms
headerPosition: "sticky"to"fixed"for ExpandedHeader components. The implementation:
- Uses optional chaining to safely check nested properties
- Follows the standard migration pattern of direct prop mutation
- Handles only the specific case that needs transformation
</review_comment_end>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/directory/Directory.tsx (1)
90-91: Full‑width slot styling is appropriate hereUsing
width: "100%"onSiteNameSlotandTitleSlotmatches the centered column layout of thePageSectionand should help keep editor vs. live rendering consistent. No functional concerns from this change.If you want to avoid repeating inline style and keep things a bit cleaner, you could factor a shared style object:
const headingSlotStyle = { height: "auto", width: "100%" }; // ... <slots.SiteNameSlot style={headingSlotStyle} /> <slots.TitleSlot style={headingSlotStyle} />But this is purely optional and the current code is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/directory/Directory.tsx(1 hunks)
⏰ 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). (4)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: create-dev-release
- GitHub Check: semgrep/ci
4 participants
Uh oh!
There was an error while loading. Please reload this page.