feat: promo variants #958
feat: promo variants #958
Conversation
|
| Key | Languages Removed |
|---|---|
fields.imageOrientation |
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 zh-TW |
fields.mediaOrientation |
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 zh-TW |
WalkthroughRefactors PromoSection into a variant-driven implementation and adds four variants: classic, immersive, spotlight, and compact. Introduces PromoContent, PromoMedia, PromoEmptyState, and per-variant components (ClassicPromo, ImmersivePromo, SpotlightPromo, CompactPromo). Expands PromoStyles and PromoData (adds variant, desktopImagePosition, mobileImagePosition, containerAlignment, imageHeight, and backgroundImage). Adds migration 0050_promo_variants and registers it. Adds locale keys (containerAlignment, bottom_direction, top_direction, promoBanner) across locales, updates tests and testing harness, and minor edits to Video and HeroSection. Sequence Diagram(s)sequenceDiagram
autonumber
participant Editor as Visual Editor (client)
participant Migration as MigrationRegistry / 0050_promo_variants
participant Promo as PromoSection
participant Resolver as resolveFields / resolveData
participant Selector as Variant Selector
participant Classic as ClassicPromo
participant Immersive as ImmersivePromo
participant Spotlight as SpotlightPromo
participant Compact as CompactPromo
participant Empty as PromoEmptyState
participant Content as PromoContent
participant Media as PromoMedia
Editor->>Promo: mount with props & locale
Promo->>Migration: apply propTransformation
Migration-->>Promo: transformed props (variant, positions, backgroundImage)
Promo->>Resolver: resolve fields & data for variant
Resolver-->>Promo: resolved fields/data
Promo->>Selector: read styles.variant
alt styles.variant == "classic"
Selector->>Classic: render ClassicPromo (slots, styles)
Classic->>Media: render PromoMedia
Classic->>Content: render PromoContent
else styles.variant == "immersive"
Selector->>Immersive: render ImmersivePromo
Immersive->>Promo: resolve backgroundImage & overlay
Immersive->>Content: render PromoContent
else styles.variant == "spotlight"
Selector->>Spotlight: render SpotlightPromo
Spotlight->>Content: render PromoContent
else styles.variant == "compact"
Selector->>Compact: render CompactPromo
Compact->>Media: render responsive PromoMedia
Compact->>Content: render PromoContent
else editor && no mapped data
Selector->>Empty: render PromoEmptyState
Empty-->>Editor: show editor placeholder
end
Content-->>Editor: apply alignment/layout classes
Media-->>Editor: render image/video
Promo-->>Editor: composed DOM
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (15)
starter/localData/dev-location-stream__en__a5ac5ff0d6330870779568c3bc865bc7.json (1)
457-457: Simplify redundant bold markup.The HTML uses nested
<b><strong style="font-weight: bold;">tags. Since<strong>already provides bold styling and semantic emphasis, the outer<b>tag and the inlinefont-weight: bold;style are redundant.🔎 Suggested simplification
- "html": "<p dir=\"ltr\" style=\"font-size: 14.67px; font-weight: 400; line-height: 18.67px; color: rgb(0, 0, 0); margin: 0; padding: 3px 2px 3px 2px; position: relative;\"><span>Our out-of-this-world </span><b><strong style=\"font-weight: bold;\">burgers</strong></b><span> and</span><b><strong style=\"font-weight: bold;\"> fresh salads</strong></b><span> are a flavor journey you won't forget. Explore a galaxy of taste, where every ingredient composes a symphony of flavors. Come visit us for a stellar dining experience!</span></p>" + "html": "<p dir=\"ltr\" style=\"font-size: 14.67px; font-weight: 400; line-height: 18.67px; color: rgb(0, 0, 0); margin: 0; padding: 3px 2px 3px 2px; position: relative;\"><span>Our out-of-this-world </span><strong>burgers</strong><span> and </span><strong>fresh salads</strong><span> are a flavor journey you won't forget. Explore a galaxy of taste, where every ingredient composes a symphony of flavors. Come visit us for a stellar dining experience!</span></p>"starter/localData/dev-location-stream__zh_hans_hk__cbafb9cd1c3e63d9814e236ba9181377.json (1)
448-448: Data update aligns with promo standardization.The HTML simplification in the promo description is consistent with the PR's broader PromoSection refactoring objectives.
Optional: Consider removing redundant bold markup
The
<b><strong>double-wrapping is semantically redundant since both tags produce bold text. You could simplify to just<strong>for slightly cleaner markup:-<b><strong style="font-weight: bold;">burgers</strong></b> +<strong style="font-weight: bold;">burgers</strong>Same for "fresh salads". This is a minor data quality improvement and can be deferred or skipped if the HTML is generated by a rich text editor.
starter/localData/dev-location-stream__en__cbafb9cd1c3e63d9814e236ba9181377.json (1)
448-448: Consider simplifying redundant HTML markup.The HTML uses
<b><strong style="font-weight: bold;">which is redundant—<strong>alone provides semantic bold styling. The inlinefont-weight: bold;style is also unnecessary.🔎 Proposed simplification
- "html": "<p dir=\"ltr\" style=\"font-size: 14.67px; font-weight: 400; line-height: 18.67px; color: rgb(0, 0, 0); margin: 0; padding: 3px 2px 3px 2px; position: relative;\"><span>Our out-of-this-world </span><b><strong style=\"font-weight: bold;\">burgers</strong></b><span> and</span><b><strong style=\"font-weight: bold;\"> fresh salads</strong></b><span> are a flavor journey you won't forget. Explore a galaxy of taste, where every ingredient composes a symphony of flavors. Come visit us for a stellar dining experience!</span></p>" + "html": "<p dir=\"ltr\" style=\"font-size: 14.67px; font-weight: 400; line-height: 18.67px; color: rgb(0, 0, 0); margin: 0; padding: 3px 2px 3px 2px; position: relative;\"><span>Our out-of-this-world </span><strong>burgers</strong><span> and</span><strong> fresh salads</strong><span> are a flavor journey you won't forget. Explore a galaxy of taste, where every ingredient composes a symphony of flavors. Come visit us for a stellar dining experience!</span></p>"packages/visual-editor/locales/nb/visual-editor.json (1)
299-299: Consider verifying translation consistency for directional terms.There's a minor asymmetry in the translation approach:
"bottom_direction": "Nederst"uses a superlative form (bottommost), distinct from"bottom": "Bunn"(line 298)"top_direction": "Topp"uses the same term as"top": "Topp"(line 393)While both translations are valid Norwegian, consider verifying whether this difference is intentional or if
"bottom_direction"should use"Bunn"to maintain parallel structure with"top_direction".Also applies to: 394-394
starter/localData/dev-location-stream__en__8573da7e3961090ddb59214a1f01e59a.json (1)
459-459: LGTM! Optional: simplify redundant bold markup.The markup simplification preserves the content correctly. The nested
<b><strong>tags are functionally redundant—<strong>alone provides semantic emphasis and bold styling.Optional cleanup
- "html": "<p dir=\"ltr\" style=\"font-size: 14.67px; font-weight: 400; line-height: 18.67px; color: rgb(0, 0, 0); margin: 0; padding: 3px 2px 3px 2px; position: relative;\"><span>Our out-of-this-world </span><b><strong style=\"font-weight: bold;\">burgers</strong></b><span> and</span><b><strong style=\"font-weight: bold;\"> fresh salads</strong></b><span> are a flavor journey you won't forget. Explore a galaxy of taste, where every ingredient composes a symphony of flavors. Come visit us for a stellar dining experience!</span></p>" + "html": "<p dir=\"ltr\" style=\"font-size: 14.67px; font-weight: 400; line-height: 18.67px; color: rgb(0, 0, 0); margin: 0; padding: 3px 2px 3px 2px; position: relative;\"><span>Our out-of-this-world </span><strong style=\"font-weight: bold;\">burgers</strong><span> and </span><strong style=\"font-weight: bold;\">fresh salads</strong><span> are a flavor journey you won't forget. Explore a galaxy of taste, where every ingredient composes a symphony of flavors. Come visit us for a stellar dining experience!</span></p>"starter/src/dev.config.ts (1)
1109-1229:c_emptyPromoschema mirrorsc_examplePromoappropriatelyThe new
c_emptyPromodefinition (registryId,type.promo_section, and nestedimage/title/description/ctafields) cleanly mirrorsc_examplePromo, so it should behave as a structurally identical promo slot.If you want this field to appear with a friendly label anywhere that uses
apiNamesToDisplayNames, consider adding an entry forc_emptyPromothere (similar to other non-example custom fields), but it’s optional and consistent with howc_examplePromois currently handled.packages/visual-editor/locales/lt/visual-editor.json (1)
611-611: Translation includes slight redundancy but may be intentional.The translation
"Reklaminė reklamjuostė"(promotional banner) contains the root word "reklam-" twice, which creates mild redundancy. While"Reklamjuostė"alone might suffice, the current translation emphasizes the promotional nature explicitly, which may be intentional for clarity in the UI context.If this redundancy is unintentional, consider simplifying to
"Reklamjuostė"or an alternative like"Promo reklamjuostė".packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
2360-2361: Remove or complete the empty if block.This no-op conditional appears to be leftover debug code or an incomplete implementation. It should either be removed or completed with the intended logic.
🔎 Proposed fix
- if (name.includes("[compact] version 31 props with constant")) { - }packages/visual-editor/src/components/pageSections/PromoSection/PromoMedia.tsx (1)
32-38: Remove commented-out code.The commented
classNameprop on line 37 should be removed if it's not needed, or uncommented and properly configured if it is needed.🔎 Proposed fix
<EntityField displayName={pt("fields.media", "Media")} fieldId={data.promo.field} constantValueEnabled={data.promo.constantValueEnabled} fullHeight - // className="flex" >packages/visual-editor/src/components/pageSections/PromoSection/SpotlightPromo.tsx (1)
27-35: Remove redundant background styling.Background properties are specified both in the className (
bg-no-repeat bg-center bg-cover) and inline style. The inline styles will override the CSS utilities, making the className properties redundant.🔎 Proposed fix
<div - className="bg-no-repeat bg-center bg-cover" style={{ backgroundImage: resolvedBackgroundImage?.url ? `url(${getImageUrl(resolvedBackgroundImage.url, resolvedBackgroundImage.width, resolvedBackgroundImage.height)})` : undefined, backgroundSize: "cover", backgroundPosition: "center", backgroundRepeat: "no-repeat", }} >packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
125-129: Consider uncommenting or removing the conditionalRender comment.The commented-out
conditionalRenderinPromoVariantPropssuggests future functionality. If this is planned, consider adding a TODO comment with context; otherwise, remove the dead comment.packages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsx (2)
5-7: Trailing whitespace in template string.Line 6 has a trailing space at the end of the template string which will be included in the className.
🔎 Proposed fix
export const promoContentParentCn = (styles: PromoVariantProps["styles"]) => { - return `flex flex-col gap-y-8 sm:gap-y-10 w-full break-words ${styles.containerAlignment === "left" ? "items-start" : styles.containerAlignment === "center" ? "items-center" : "items-end"} `; + return `flex flex-col gap-y-8 sm:gap-y-10 w-full break-words ${styles.containerAlignment === "left" ? "items-start" : styles.containerAlignment === "center" ? "items-center" : "items-end"}`; };
12-15: Duplicated alignment logic between helper and component.The alignment class logic on line 14 duplicates the logic in
promoContentParentCn(line 6). Consider extracting the alignment ternary into a shared helper or reusing the existing function to avoid divergence.🔎 Proposed refactor
+const getAlignmentClass = (alignment: PromoVariantProps["styles"]["containerAlignment"]) => + alignment === "left" ? "items-start" : alignment === "center" ? "items-center" : "items-end"; + export const promoContentParentCn = (styles: PromoVariantProps["styles"]) => { - return `flex flex-col gap-y-8 sm:gap-y-10 w-full break-words ${styles.containerAlignment === "left" ? "items-start" : styles.containerAlignment === "center" ? "items-center" : "items-end"} `; + return `flex flex-col gap-y-8 sm:gap-y-10 w-full break-words ${getAlignmentClass(styles.containerAlignment)}`; }; export const PromoContent: PuckComponent<PromoVariantProps> = (props) => { const { slots, styles } = props; return ( <div - className={`flex flex-col justify-center gap-y-6 sm:gap-y-8 w-full break-words ${styles.containerAlignment === "left" ? "items-start" : styles.containerAlignment === "center" ? "items-center" : "items-end"}`} + className={`flex flex-col justify-center gap-y-6 sm:gap-y-8 w-full break-words ${getAlignmentClass(styles.containerAlignment)}`} >packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx (1)
2714-2737: Duplicate axe violation handling logic.The axe violation handling (brand color check + conditional expect) is duplicated between the initial render (lines 2714-2722) and the post-interaction check (lines 2729-2737). Consider extracting this into a helper function.
🔎 Proposed refactor
+const assertNoAxeViolations = (results: any, props: any, testName: string) => { + if (isBrandColorTest(props)) { + console.warn(`IGNORING axe violations for brand color test: ${testName}`, results); + } else { + expect(results).toHaveNoViolations(); + } +}; // Then use it: - const results = await axe(container); - if (isBrandColorTest(props)) { - console.warn( - `IGNORING axe violations for brand color test: ${name}`, - results - ); - } else { - expect(results).toHaveNoViolations(); - } + const results = await axe(container); + assertNoAxeViolations(results, props, name);packages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsx (1)
18-22: Unnecessary optional chaining ondata.Since
datais destructured frompropson line 14 and is a required property ofPromoVariantProps, the optional chaining ondata?.backgroundImage(line 20) is unnecessary.🔎 Proposed fix
const resolvedBackgroundImage = resolveYextEntityField( streamDocument, - data?.backgroundImage, + data.backgroundImage, locale );
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
packages/visual-editor/locales/cs/visual-editor.jsonpackages/visual-editor/locales/da/visual-editor.jsonpackages/visual-editor/locales/de/visual-editor.jsonpackages/visual-editor/locales/en-GB/visual-editor.jsonpackages/visual-editor/locales/en/visual-editor.jsonpackages/visual-editor/locales/es/visual-editor.jsonpackages/visual-editor/locales/et/visual-editor.jsonpackages/visual-editor/locales/fi/visual-editor.jsonpackages/visual-editor/locales/fr/visual-editor.jsonpackages/visual-editor/locales/hr/visual-editor.jsonpackages/visual-editor/locales/hu/visual-editor.jsonpackages/visual-editor/locales/it/visual-editor.jsonpackages/visual-editor/locales/ja/visual-editor.jsonpackages/visual-editor/locales/lt/visual-editor.jsonpackages/visual-editor/locales/lv/visual-editor.jsonpackages/visual-editor/locales/nb/visual-editor.jsonpackages/visual-editor/locales/nl/visual-editor.jsonpackages/visual-editor/locales/pl/visual-editor.jsonpackages/visual-editor/locales/pt/visual-editor.jsonpackages/visual-editor/locales/ro/visual-editor.jsonpackages/visual-editor/locales/sk/visual-editor.jsonpackages/visual-editor/locales/sv/visual-editor.jsonpackages/visual-editor/locales/tr/visual-editor.jsonpackages/visual-editor/locales/zh-TW/visual-editor.jsonpackages/visual-editor/locales/zh/visual-editor.jsonpackages/visual-editor/src/components/categories/PageSectionCategory.tsxpackages/visual-editor/src/components/contentBlocks/Video.tsxpackages/visual-editor/src/components/migrations/0049_promo_variants.tspackages/visual-editor/src/components/migrations/migrationRegistry.tspackages/visual-editor/src/components/pageSections/HeroSection.test.tsxpackages/visual-editor/src/components/pageSections/HeroSection.tsxpackages/visual-editor/src/components/pageSections/PromoSection.test.tsxpackages/visual-editor/src/components/pageSections/PromoSection/ClassicPromo.tsxpackages/visual-editor/src/components/pageSections/PromoSection/CompactPromo.tsxpackages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoMedia.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/PromoSection/SpotlightPromo.tsxpackages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsxpackages/visual-editor/src/components/pageSections/heroVariants/HeroContent.tsxpackages/visual-editor/src/components/pageSections/index.tspackages/visual-editor/src/components/testing/componentTests.setup.tspackages/visual-editor/src/docs/ai/components.d.tspackages/visual-editor/src/docs/components.mdstarter/localData/dev-location-stream__en__8573da7e3961090ddb59214a1f01e59a.jsonstarter/localData/dev-location-stream__en__a5ac5ff0d6330870779568c3bc865bc7.jsonstarter/localData/dev-location-stream__en__aa6fc9407fdca53fc4133ee078b93231.jsonstarter/localData/dev-location-stream__en__cbafb9cd1c3e63d9814e236ba9181377.jsonstarter/localData/dev-location-stream__en__d7f52f17aa2a321e60e8c1a8fae53637.jsonstarter/localData/dev-location-stream__es__cbafb9cd1c3e63d9814e236ba9181377.jsonstarter/localData/dev-location-stream__zh_hans_hk__cbafb9cd1c3e63d9814e236ba9181377.jsonstarter/src/dev.config.ts
💤 Files with no reviewable changes (1)
- packages/visual-editor/src/components/pageSections/PromoSection.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-12T20:17:37.033Z
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:37.033Z
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/0049_promo_variants.tspackages/visual-editor/src/components/migrations/migrationRegistry.ts
📚 Learning: 2025-11-06T14:55:12.395Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 862
File: packages/visual-editor/src/utils/schema/resolveSchema.ts:118-135
Timestamp: 2025-11-06T14:55:12.395Z
Learning: In `packages/visual-editor/src/utils/schema/resolveSchema.ts`, the `OpeningHoursSchema` and `PhotoGallerySchema` functions from `yext/pages-components` contain internal type validation and handle invalid inputs gracefully (returning empty objects or undefined) rather than throwing TypeErrors, so no pre-validation guards are needed before calling them.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
🧬 Code graph analysis (7)
packages/visual-editor/src/components/pageSections/PromoSection/SpotlightPromo.tsx (3)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
PromoVariantProps(125-129)packages/visual-editor/scripts/generateFontRegistry.js (1)
data(42-42)packages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsx (2)
promoContentParentCn(5-7)PromoContent(9-25)
packages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsx (1)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
PromoVariantProps(125-129)
packages/visual-editor/src/components/pageSections/PromoSection/ClassicPromo.tsx (3)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
PromoVariantProps(125-129)packages/visual-editor/src/components/pageSections/PromoSection/PromoMedia.tsx (1)
PromoMedia(6-57)packages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsx (1)
PromoContent(9-25)
packages/visual-editor/src/components/pageSections/PromoSection/PromoMedia.tsx (3)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (2)
PromoData(36-50)PromoStyles(52-89)packages/visual-editor/src/components/pageSections/index.ts (2)
PromoData(55-55)PromoStyles(56-56)packages/visual-editor/scripts/generateFontRegistry.js (1)
data(42-42)
packages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsx (2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
PromoVariantProps(125-129)packages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsx (2)
promoContentParentCn(5-7)PromoContent(9-25)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
axe(77-81)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx (4)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
ComponentTest(109-121)packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
PromoSection(292-643)packages/visual-editor/src/components/pageSections/index.ts (1)
PromoSection(53-53)packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
migrationRegistry(57-107)
🪛 Biome (2.1.2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx
[error] 15-15: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
[error] 433-435: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 448-455: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
⏰ 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 (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: create-dev-release
- GitHub Check: semgrep/ci
🔇 Additional comments (59)
starter/localData/dev-location-stream__es__cbafb9cd1c3e63d9814e236ba9181377.json (1)
448-448: LGTM!The update to the example promo description is appropriate for the promo variants refactoring. The HTML is valid and properly escaped for JSON serialization. Since this is dev/test data, the change poses no risk to the system.
packages/visual-editor/locales/zh-TW/visual-editor.json (1)
196-196: LGTM! Localization additions support the new promo variants feature.The added localization keys (
containerAlignment,bottom_direction,top_direction,promoBanner) are correctly placed in alphabetical order and align with the PR's introduction of multi-variant promo layouts. The JSON structure is valid and the Traditional Chinese translations are provided.Also applies to: 299-299, 395-395, 612-612
packages/visual-editor/locales/hr/visual-editor.json (1)
196-196: LGTM! Localization additions are structurally sound.The JSON structure is valid, and the added translations are properly positioned within their respective sections. The Croatian translations appear contextually appropriate:
- "Usklađivanje spremnika" (container alignment)
- "Dno" (bottom direction)
- "Vrh" (top direction)
- "Promo banner" (kept in English, standard for marketing terms)
If these translations haven't been reviewed by native Croatian speakers or professional translators, consider having them verified to ensure linguistic accuracy and naturalness, especially since this is part of a broader 22-locale update.
Also applies to: 299-299, 394-394, 611-611
packages/visual-editor/locales/es/visual-editor.json (3)
195-195: LGTM! Accurate Spanish translation for container alignment.The translation "Alineación de contenedores" correctly conveys "container alignment" in Spanish and is properly placed within the fields section.
298-298: LGTM! Direction translations are accurate.Both "bottom_direction" → "Abajo" and "top_direction" → "Arriba" are correct Spanish translations and follow the consistent naming pattern established by other direction options in the file (center_direction, left_direction, right_direction).
Also applies to: 393-393
610-610: LGTM! Proper translation for promotional banner.The translation "Banner promocional" is accurate and appropriately placed alongside related promotional content keys.
packages/visual-editor/locales/zh/visual-editor.json (4)
196-196: LGTM! Translation addition aligns with promo variants feature.The
containerAlignmentfield addition with translation "容器对齐" (Container alignment) is properly structured and consistent with the new promo variants functionality being introduced across all locale files.
299-299: LGTM! Directional option translation is accurate.The
bottom_directiontranslation "底部" is correct and appropriately placed within the options block for directional controls.
394-394: LGTM! Directional option translation is accurate.The
top_directiontranslation "顶部" is correct and complements thebottom_directionoption for directional controls in the promo variants feature.
611-611: LGTM! Promotional banner translation is appropriate.The
promoBannerfield addition with translation "促销横幅" (Promotional banner) is properly structured and aligns with the new promotional banner functionality in the promo variants feature.packages/visual-editor/locales/sv/visual-editor.json (1)
197-197: Recommend verification of Swedish translations by a localization expert.The new translation keys follow the existing structural patterns in the file, which is good. However, please ensure these Swedish translations are reviewed by a native speaker or localization expert for accuracy:
containerAlignment: "Behållaruppriktning"bottom_direction: "Nederst"top_direction: "Topp"promoBanner: "Kampanjbanner"Note: The term "containerAlignment" uses "uppriktning" while the existing "alignment" key uses "inriktning". If this distinction is intentional (different types of alignment), that's fine—but worth confirming.
Since this is part of a coordinated update across 22 languages, consider running a similar verification pass for all locale files in this PR.
Also applies to: 300-300, 395-395, 612-612
packages/visual-editor/locales/pl/visual-editor.json (4)
197-197: LGTM! Translation addition looks correct.The
containerAlignmentfield translation is properly structured and the Polish translation "Wyrównanie kontenera" is appropriate for "Container alignment."
300-300: LGTM! Directional translation is appropriate.The
bottom_directionoption translation "Dół" correctly represents "Bottom" in Polish and is consistent with other directional options in the file.
395-395: LGTM! Directional translation is appropriate.The
top_directionoption translation "Góra" correctly represents "Top" in Polish and pairs well with thebottom_directionaddition.
612-612: LGTM! Promotional banner translation is correct.The
promoBannertranslation "Baner promocyjny" is the standard Polish term for promotional banner and is properly positioned in the root-level keys.packages/visual-editor/locales/nb/visual-editor.json (1)
196-196: LGTM! Translations are grammatically correct.The Norwegian translations for the new keys are accurate:
- "containerAlignment" → "Beholderjustering" (container alignment)
- "bottom_direction" → "Nederst" (bottommost)
- "top_direction" → "Topp" (top)
- "promoBanner" → "Kampanjebanner" (campaign banner)
Also applies to: 299-299, 394-394, 611-611
packages/visual-editor/locales/en-GB/visual-editor.json (1)
195-195: LGTM! Clean localization additions across all locale files.The new localization keys are properly formatted, grammatically correct, and appropriately placed within the JSON structure. All four keys (containerAlignment, bottom_direction, top_direction, promoBanner) have been consistently added across all 25 locale files.
packages/visual-editor/locales/nl/visual-editor.json (5)
196-196: LGTM! Container alignment translation added correctly.The Dutch translation "Uitlijning van containers" is appropriate and the key is correctly positioned alphabetically within the fields section.
299-299: LGTM! Bottom direction translation added correctly.The Dutch translation "Onder" accurately represents "bottom" direction and is positioned appropriately within the options section.
394-394: LGTM! Top direction translation added correctly.The Dutch translation "Boven" accurately represents "top" direction and maintains consistency with the
bottom_directionkey pattern.
611-611: LGTM! Promo banner translation added correctly.The Dutch translation "Promobanner" follows standard Dutch compound word formation and is correctly positioned alphabetically at the root level.
196-196: Consistency across all 25 locale files verified. All new keys (containerAlignment,bottom_direction,top_direction,promoBanner) are present in every language file, and removed keys (imageOrientation,mediaOrientation) have been cleanly removed from all locales without exceptions.packages/visual-editor/locales/ro/visual-editor.json (1)
196-196: Verify translation consistency across all locale files.The AI summary indicates that 22+ locale files were updated with these same keys. Given the empty string issue found in
bottom_directionfor the Romanian locale, please verify that all other locale files have proper translations for all four new keys (containerAlignment,bottom_direction,top_direction,promoBanner).#!/bin/bash # Description: Check all locale files for empty or missing translations in the new keys echo "=== Checking for empty bottom_direction values ===" fd -e json . packages/visual-editor/locales/ --exec grep -l '"bottom_direction": ""' echo -e "\n=== Checking all locale files have the new keys ===" for key in "containerAlignment" "bottom_direction" "top_direction" "promoBanner"; do echo -e "\nKey: $key" fd -e json . packages/visual-editor/locales/ --exec sh -c 'file="{}"; if grep -q "\"'"$key"'\"" "$file"; then echo "✓ $file"; else echo "✗ MISSING: $file"; fi' done echo -e "\n=== Sample translations for bottom_direction across locales ===" fd -e json . packages/visual-editor/locales/ --exec sh -c 'file="{}"; result=$(grep "bottom_direction" "$file" 2>/dev/null); if [ -n "$result" ]; then echo "$file: $result"; fi'Also applies to: 299-299, 394-394, 611-611
packages/visual-editor/locales/lv/visual-editor.json (1)
196-196: Remove unused locale keys or implement their references.The locale keys
containerAlignmentandpromoBannerare properly referenced in the promo section components. However,bottom_directionandtop_directionare added to the Latvian locale file but have no corresponding references in the codebase. Remove these unused keys or implement their usage in the component code if intended for future features.starter/src/dev.config.ts (1)
446-449: Expression entry forc_emptyPromois consistent and correctAdding
c_emptyPromowithfullObject: truealongsidec_examplePromomatches existing patterns and aligns with the schema block below. I don’t see any issues here.packages/visual-editor/locales/lt/visual-editor.json (2)
1-748: LGTM: JSON structure is valid and changes align with PR objectives.The localization updates correctly support the new PromoSection variant architecture by:
- Adding
containerAlignmentsupport for the new alignment controls- Introducing directional labels (
bottom_direction,top_direction) for layout options- Adding the
promoBannerlabel for the promotional banner feature- Removing legacy orientation keys (per the AI summary)
The JSON structure is well-formed and the changes are consistent with the broader refactoring effort across all 22 locales.
196-196: Fix inconsistent Lithuanian translation for directional options.The Lithuanian locale has an inconsistency in the new directional translations:
"bottom"and"bottom_direction"both use"Apačia"(consistent)"top"uses"Viršus"(masculine) while"top_direction"uses"Viršutinė"(feminine adjective)This inconsistency is unique to Lithuanian—other locales either use the same translation for both (English, Spanish, French, etc.) or use distinct noun/directional forms consistently. The current mixed approach suggests a translation error. Update
"top_direction"to use"Viršus"to match the pattern established by"bottom"and"bottom_direction", unless the feminine form is intentional for a specific UI context.packages/visual-editor/locales/ja/visual-editor.json (2)
196-196: LGTM: containerAlignment translation is consistent.The Japanese translation "コンテナの位置合わせ" for
containerAlignmentaligns well with other alignment-related translations in the file.
611-611: Inconsistent with AI summary: only onepromoBannerinstance found.The AI summary states that
promoBannerwas "added in two separate root-level sections related to product areas, duplicating insertion points," but only one instance at line 611 is visible in the provided code. Please verify whether:
- The duplicate exists in another file not included in this review
- The summary is incorrect
- One insertion was missed during the changes
packages/visual-editor/src/components/contentBlocks/Video.tsx (4)
9-11: LGTM! Internal className prop for flexibility.The optional
classNameprop marked as@internalprovides flexibility for customizing the video styling while maintaining a sensible default. The JSDoc annotation clearly indicates this is not part of the public API.
40-40: LGTM! Consistent dragRef on editing placeholder.The placeholder div correctly attaches
dragReffor drag-and-drop support in editing mode when no video is configured. Theh-20 mt-8classes provide appropriate placeholder dimensions and spacing.
51-51: Theinline: trueflag removes the component's wrapping element and is used to create advanced CSS layouts. When inline is true, you must specify which item is draggable via the puck.dragRef prop.Verify that the Video component's render function properly applies
puck.dragRefto the appropriate element. Ifinline: trueis set butdragRefis not used in the render output, this creates a configuration error and the component will not be draggable. Check the Video component's render method to ensure dragRef is applied where needed for the intended layout behavior.
28-28: Layout behavior is correct—no changes needed.The dragRef wrapper pattern with h-full w-full is the documented approach for Puck's drag-and-drop functionality. The wrapper creates the drag target area without affecting the VideoAtom's responsive layout constraints. Child width classes like lg:w-4/5 and mx-auto centering work as intended inside this wrapper.
packages/visual-editor/locales/sk/visual-editor.json (2)
197-197: LGTM - New translation key added correctly.The
containerAlignmentkey is properly structured and alphabetically placed. The Slovak translation "Zarovnanie kontajnera" appears appropriate for "Container alignment".
300-300: LGTM - Directional translation keys appropriately distinguish from positional terms.The new
bottom_direction("Dole" = downward) andtop_direction("Hore" = upward) translations correctly differentiate from the existing positional termsbottom("Spodná časť" = bottom part) andtop("Vrchol" = top/peak). This semantic distinction aligns with the PR's architectural shift from orientation-based to position/direction-based controls.Also applies to: 395-395
packages/visual-editor/locales/hu/visual-editor.json (1)
1-748: Migration of deprecated orientation properties is complete and properly implemented.Verification confirms that the
imageOrientationfield has been successfully removed from theHeroStylestype definition and properly migrated. The migration file0017_hero_variants.tscorrectly transforms legacy data by mappingimageOrientationto the newdesktopImagePositionfield. No production code depends on the removed field—only migration logic and test fixtures reference it. ThemediaOrientationfield was not found in the codebase.Note: The Hungarian locale file contains only UI translation strings and does not include component configuration keys. The actual deprecation was handled in the TypeScript type system and migration logic.
packages/visual-editor/locales/fr/visual-editor.json (1)
195-195: LGTM - Consistent localization updates.The French locale additions (containerAlignment, bottom_direction, top_direction, promoBanner) align with the broader promo variants refactor across all locales and support the new PromoSection data model.
Also applies to: 298-298, 393-393, 611-611
packages/visual-editor/locales/et/visual-editor.json (1)
196-196: LGTM - Consistent localization updates.The Estonian locale additions mirror the pattern across all locales in this PR and support the new PromoSection variant architecture.
Also applies to: 299-299, 394-394, 611-611
packages/visual-editor/locales/da/visual-editor.json (1)
202-202: LGTM - Consistent localization updates.The Danish locale additions are consistent with the promo variants refactor across all 22 languages.
Also applies to: 306-306, 401-401, 618-618
packages/visual-editor/locales/de/visual-editor.json (1)
200-200: LGTM - Consistent localization updates.The German locale additions align with the broader localization expansion for promo variants.
Also applies to: 304-304, 399-399, 616-616
starter/localData/dev-location-stream__en__d7f52f17aa2a321e60e8c1a8fae53637.json (1)
447-447: LGTM - Test data cleanup.The HTML content simplification removes inline styling while preserving the semantic content, aligning with the cleaner promo rendering approach.
packages/visual-editor/src/components/categories/PageSectionCategory.tsx (1)
30-30: LGTM - Module reorganization.The import path update reflects the PromoSection refactor into a dedicated directory structure, consistent with the new variant architecture (ClassicPromo, ImmersivePromo, SpotlightPromo, CompactPromo).
packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
50-50: LGTM - Correct migration registration.The promoVariants migration is properly imported and added to the registry, following the established pattern for extending the migration pipeline.
Based on learnings, the migration index allocation is appropriate.
Also applies to: 106-106
packages/visual-editor/src/components/pageSections/index.ts (1)
53-57: LGTM!The export path update correctly reflects the new PromoSection folder structure with variant components.
packages/visual-editor/src/components/pageSections/heroVariants/HeroContent.tsx (1)
17-18: LGTM!The conditional width logic correctly omits
w-fullfor the compact variant, allowing proper two-column layout behavior while maintaining full width for other variants.packages/visual-editor/src/components/pageSections/HeroSection.tsx (2)
541-541: LGTM!Simplified className for compact variant image correctly removes the responsive width breakpoint, aligning with the updated compact layout behavior.
668-678: LGTM!The type widening to
(string | undefined)[]and the falsy path guard enable cleaner conditional field updates, particularly useful when paths are conditionally included based on component state.packages/visual-editor/src/components/pageSections/PromoSection/CompactPromo.tsx (2)
31-52: Verify duplicate content rendering impact.
PromoContentis rendered twice: once in the mobile container (lines 38) and once in the desktop container (line 51), with visibility toggled via CSS. This is a common responsive pattern but may have implications:
- Accessibility: Screen readers may read content twice
- SEO: Duplicate content in DOM
- Performance: Slot components render twice
Consider using a single
PromoContentinstance with responsive styling, or verify this pattern is intentional and acceptable for the use case.
1-67: Well-structured responsive layout.The component correctly implements the compact variant with conditional image positioning for desktop/mobile viewports. The use of
themeManagerCnandpromoContentParentCnhelpers maintains consistency with other promo variants.packages/visual-editor/src/components/pageSections/PromoSection/SpotlightPromo.tsx (1)
13-59: Component implementation looks correct.The SpotlightPromo variant properly resolves the background image, applies container alignment, and renders PromoContent within a styled Background wrapper. The use of
minHeightwithstyles.imageHeightprovides appropriate sizing control.packages/visual-editor/src/components/pageSections/PromoSection/ClassicPromo.tsx (1)
7-38: LGTM!Clean implementation of the classic promo variant. The responsive visibility logic for image positioning is consistent with the CompactPromo pattern, and the component properly composes PageSection, PromoMedia, and PromoContent.
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
591-628: Well-implemented variant rendering with empty state handling.The render function cleanly handles variant selection and provides appropriate empty state feedback in editor mode when mapped entity fields are empty. The pattern mirrors the HeroSection approach effectively.
packages/visual-editor/src/docs/ai/components.d.ts (1)
698-733: LGTM! Type definitions are well-structured and consistent with HeroSection patterns.The new
PromoStylesinterface correctly mirrors theHeroStylesvariant architecture. One minor observation:imageHeightis required here (number) while inHeroStyles(line 656) it's optional (number?). This appears intentional since the default is documented as 500px.packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx (1)
64-2636: Comprehensive test coverage for all PromoSection variants and migration paths.Excellent test coverage including:
- All four variants (classic, immersive, spotlight, compact)
- Multiple migration versions (0, 5, 16, 24, 30, 48, 49)
- Entity values, constant values, and mixed value scenarios
- Empty state handling
- Video and image media types
- Accessibility testing with axe
The tests will help ensure the migration from the old orientation-based system to the new variant system works correctly.
packages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsx (1)
13-56: Well-structured immersive variant implementation.The component properly:
- Resolves background images with locale awareness
- Applies a semi-transparent dark overlay for text readability when an image is present
- Falls back gracefully to
backgroundColors.background1when no image exists- Uses
aria-labelfor accessibility- Respects the
imageHeightstyle for minimum heightThe pattern aligns well with the HeroSection immersive variant approach.
packages/visual-editor/src/docs/components.md (1)
682-699: Documentation accurately reflects the new PromoSection API.The updated tables correctly document:
- New
data.backgroundImagefield- New style props:
variant,containerAlignment,desktopImagePosition,mobileImagePosition,imageHeight- Appropriate default values matching the type definitions
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx (1)
34-91: LGTM on the responsive layout restructure.The two-column layout implementation with conditional visibility classes correctly handles all combinations of
desktopImagePositionandmobileImagePosition. The dynamic padding calculation using CSS variables ensures proper alignment with content width constraints.packages/visual-editor/locales/tr/visual-editor.json (2)
196-196: LGTM! Translation appears correct.The Turkish translation "Konteyner Hizalaması" for
containerAlignmentfollows the established pattern and appears linguistically appropriate for the UI context.
299-299: LGTM! Consistent with existing translations.The translation "Alt" for
bottom_directioncorrectly matches the existingbottomtranslation, maintaining consistency within the locale file.
packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoMedia.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
2377-2381: Fix inconsistent logging in post-interaction branch.Line 2380 logs the full
resultsobject, while Line 2365 correctly logsresults.violations. These should be consistent.🔎 Proposed fix
if (isBrandColorTest(props) && results.violations.length) { console.warn( `IGNORING axe violations for brand color test: ${name}`, - results + results.violations );
♻️ Duplicate comments (4)
packages/visual-editor/src/components/migrations/0049_promo_variants.ts (1)
7-24: Mutation of original props and potential null reference still present.The issues flagged in the previous review have not been addressed:
- Line 9 mutates the original
propsobject viadelete props.styles?.orientation- The shallow copy on line 11 means
newProps.styles === props.styles, so assignments on lines 13-22 also mutate the original- If
stylesis undefined, lines 13-22 will throw a runtime error🔎 Proposed fix with proper immutability
export const promoVariants: Migration = { PromoSection: { action: "updated", propTransformation: (props) => { const orientation = props.styles?.orientation; - delete props.styles?.orientation; - - const newProps = { ...props }; - - newProps.styles.variant = "classic"; - newProps.styles.desktopImagePosition = orientation; - newProps.styles.mobileImagePosition = "top"; - newProps.styles.imageHeight = 500; - newProps.styles.containerAlignment = "left"; - newProps.styles.backgroundImage = { - field: "", - constantValue: getRandomPlaceholderImage({ width: 1440, height: 900 }), - constantValueEnabled: true, - }; + const { orientation: _, ...restStyles } = props.styles || {}; + + const newProps = { + ...props, + styles: { + ...restStyles, + variant: "classic", + desktopImagePosition: orientation, + mobileImagePosition: "top", + imageHeight: 500, + containerAlignment: "left", + }, + data: { + ...props.data, + backgroundImage: { + field: "", + constantValue: getRandomPlaceholderImage({ width: 1440, height: 900 }), + constantValueEnabled: true, + }, + }, + }; return newProps; }, }, };Note:
backgroundImageshould likely be underdatarather thanstylesbased on thePromoDatainterface in PromoSection.tsx (line 49).packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (2)
430-468: Add explicit fallthrough annotations to silence linter errors.The switch statement uses intentional fallthrough (compact → classic, immersive → spotlight), which triggers Biome lint errors. Add
/* falls through */comments to make the intent explicit.🔎 Proposed fix using explicit fallthrough comments
switch (data.props.styles.variant) { case "compact": { // compact should remove the same fields removed by classic } + /* falls through */ case "classic": { fields = updateFields( // ... ); break; } case "immersive": { fields = updateFields( // ... ); // immersive should also remove the fields removed by spotlight } + /* falls through */ case "spotlight": {
532-536: Use a ternary conditional to handle empty field values properly.When
data.props?.data?.promo.fieldis an empty string, the current code produces".image"instead of an empty string. The|| ""fallback doesn't work because".image"is a truthy value.This issue appears in both line 533 and line 585.
🔎 Proposed fix
- field: data.props?.data?.promo.field + ".image" || "", + field: data.props?.data?.promo.field ? data.props?.data?.promo.field + ".image" : "",Apply this fix to both occurrences (lines 533 and 585).
Also applies to: 584-588
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx (1)
46-46: Duplicate ID issue persists.Both
CompactHeroImageinstances useid={id + "-image"}, creating duplicate DOM IDs. This was previously flagged and the suggested fix (using unique suffixes like-leftand-right) has not yet been applied.Also applies to: 82-82
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (32)
packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] version 4 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] version 8 with non-default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [compact] version 17 props using constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [compact] version 31 props with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [compact] version 31 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [classic] version 49 props with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [classic] version 49 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [compact] version 49 with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [compact] version 49 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [compact] version 49 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [immersive] version 49 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] default props with empty document.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] version 0 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] 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/PromoSection/[desktop] version 16 props with missing ctaType.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] 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/PromoSection/[desktop] version 24 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] version 30 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] version 48 props with translatable image and CTA.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] version 5 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [classic] version 49 props with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [compact] version 49 with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [compact] version 49 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [classic] version 49 props with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [compact] version 49 with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [compact] version 49 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [compact] version 49 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [immersive] version 49 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [spotlight] version 49 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[xl] [compact] version 49 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (7)
packages/visual-editor/src/components/migrations/0049_promo_variants.tspackages/visual-editor/src/components/pageSections/HeroSection.test.tsxpackages/visual-editor/src/components/pageSections/PromoSection/CompactPromo.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsx
- packages/visual-editor/src/components/pageSections/PromoSection/CompactPromo.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-15T20:59:25.571Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 943
File: packages/visual-editor/src/components/contentBlocks/image/Image.tsx:175-191
Timestamp: 2025-12-15T20:59:25.571Z
Learning: In the ImageWrapper component at packages/visual-editor/src/components/contentBlocks/image/Image.tsx, when an Image is wrapped in a MaybeLink, the aria-label should be omitted because the image's alt text serves as the accessible name for the link. Adding an aria-label would override the image's alt text.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-11-06T14:55:12.395Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 862
File: packages/visual-editor/src/utils/schema/resolveSchema.ts:118-135
Timestamp: 2025-11-06T14:55:12.395Z
Learning: In `packages/visual-editor/src/utils/schema/resolveSchema.ts`, the `OpeningHoursSchema` and `PhotoGallerySchema` functions from `yext/pages-components` contain internal type validation and handle invalid inputs gracefully (returning empty objects or undefined) rather than throwing TypeErrors, so no pre-validation guards are needed before calling them.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
📚 Learning: 2025-12-12T20:17:37.033Z
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:37.033Z
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/0049_promo_variants.ts
📚 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/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
🧬 Code graph analysis (1)
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx (2)
packages/visual-editor/src/components/atoms/index.ts (1)
Background(16-16)packages/visual-editor/src/components/pageSections/heroVariants/HeroContent.tsx (2)
heroContentParentCn(12-19)HeroContent(21-89)
🪛 Biome (2.1.2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
[error] 431-433: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 446-453: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
⏰ 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). (2)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (12)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx (5)
1-18: LGTM! Well-organized test imports.The imports cover all necessary testing utilities, component exports, and Puck integration for comprehensive component testing.
20-62: Good test fixture with realistic data.The
promoDatafixture provides comprehensive mock data including CTA, rich text description, image with multiple thumbnail sizes, and title - all necessary for thorough testing of the PromoSection variants.
2486-2500: Reasonable approach for brand color accessibility exceptions.The allowlist pattern for brand-color background tests that may have accessibility violations is a pragmatic solution. Brand colors are user-defined and may not always meet contrast requirements.
2502-2593: Comprehensive test harness with proper migration and rendering flow.The test suite correctly:
- Applies migrations with version tracking
- Resolves all data before rendering
- Waits for images to load
- Adds delay for video content
- Performs screenshot matching and accessibility testing
- Handles post-interaction states when defined
64-975: Extensive version coverage for migration testing.The test cases cover versions 0, 5, 16, 24, 30, 48, and 49, ensuring migration paths are properly validated. Each version has appropriate prop structures reflecting the component's evolution.
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (5)
1-35: LGTM! Clean imports with new variant components.The imports are well-organized, bringing in the new variant components (ClassicPromo, SpotlightPromo, ImmersivePromo, CompactPromo) and supporting utilities.
36-89: Well-documented interface extensions for variant system.The
PromoDataandPromoStylesinterfaces are properly documented with JSDoc comments explaining each new field's purpose and default values. The variant types and position options are clearly defined.
125-129: Good type extraction for variant props.
PromoVariantPropsprovides a clean public type for the variant components to consume. The commented-outconditionalRendersuggests future extensibility.
597-648: Clean variant routing with empty state handling.The render function properly:
- Routes to the appropriate variant component based on
styles.variant- Detects mapped fields vs constant values
- Shows empty state in editor mode when a mapped field resolves to empty
- Wraps output in analytics scope and visibility wrapper
292-426: Comprehensive default props with proper slot configuration.The
defaultPropsprovide sensible defaults for all new fields (variant, positions, alignment, imageHeight) and the slot configurations are well-structured with appropriate initial values.packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (2)
2362-2366: LGTM - Improved axe violation handling.The conditional check for
results.violations.lengthreduces noise by only logging when violations exist, and loggingresults.violationsinstead of the fullresultsobject provides cleaner output.
2206-2206: Clarify the scope and intent of XL viewport testing across variants.Only the compact variant includes
includeXLViewport: truein the test suite. Since other variants (classic, immersive, spotlight) also have responsive styling properties (e.g., desktop positioning, mobile alignment), clarify whether:
- XL viewport testing is intentionally limited to compact variant only, or
- Other variants should also be tested at XL viewport sizes for consistent coverage.
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx (1)
11-15: Fix critical hook ordering violation.
useTemplateMetadata()is called after a conditional early return, violating the Rules of Hooks. All hooks must be called unconditionally and in the same order on every render.🔎 Proposed fix
export const PromoEmptyState: React.FC<{ isEditing: boolean; backgroundStyle: BackgroundStyle | undefined; }> = (props) => { const { isEditing, backgroundStyle } = props; + const templateMetadata = useTemplateMetadata(); if (!isEditing) { return <></>; } - - const templateMetadata = useTemplateMetadata();packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
429-467: Add explicit fallthrough annotations to silence linter errors.The switch statement uses intentional fallthrough (compact → classic, immersive → spotlight), which is a valid pattern but triggers Biome lint errors. Add
/* falls through */comments to make the intent explicit.🔎 Proposed fix using explicit fallthrough comments
switch (data.props.styles.variant) { case "compact": { // compact should remove the same fields removed by classic } + /* falls through */ case "classic": { fields = updateFields( fields, [ "data.objectFields.backgroundImage", "styles.objectFields.imageHeight", ], undefined ); break; } case "immersive": { fields = updateFields( fields, ["slots.ImageSlot", "styles.objectFields.backgroundColor"], undefined ); // immersive should also remove the fields removed by spotlight } + /* falls through */ case "spotlight": {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [compact] version 49 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (12)
packages/visual-editor/locales/fi/visual-editor.jsonpackages/visual-editor/locales/ja/visual-editor.jsonpackages/visual-editor/locales/ro/visual-editor.jsonpackages/visual-editor/locales/sk/visual-editor.jsonpackages/visual-editor/src/components/migrations/0049_promo_variants.tspackages/visual-editor/src/components/pageSections/PromoSection/ClassicPromo.tsxpackages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoMedia.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/PromoSection/SpotlightPromo.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/visual-editor/src/components/pageSections/PromoSection/ClassicPromo.tsx
- packages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsx
- packages/visual-editor/src/components/pageSections/PromoSection/PromoContent.tsx
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/src/components/migrations/0049_promo_variants.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In CompactHero and CompactPromo components, duplicate IDs on image elements are intentional: conditional visibility classes (hidden/sm:block/sm:hidden) ensure only one image renders at a time based on responsive breakpoints and desktopImagePosition/mobileImagePosition settings.
📚 Learning: 2025-12-15T20:59:25.571Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 943
File: packages/visual-editor/src/components/contentBlocks/image/Image.tsx:175-191
Timestamp: 2025-12-15T20:59:25.571Z
Learning: In the ImageWrapper component at packages/visual-editor/src/components/contentBlocks/image/Image.tsx, when an Image is wrapped in a MaybeLink, the aria-label should be omitted because the image's alt text serves as the accessible name for the link. Adding an aria-label would override the image's alt text.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
📚 Learning: 2025-12-23T16:30:37.478Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In CompactHero and CompactPromo components, duplicate IDs on image elements are intentional: conditional visibility classes (hidden/sm:block/sm:hidden) ensure only one image renders at a time based on responsive breakpoints and desktopImagePosition/mobileImagePosition settings.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
🧬 Code graph analysis (3)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (6)
packages/visual-editor/src/components/pageSections/HeroSection.tsx (1)
updateFields(668-700)packages/visual-editor/src/components/pageSections/PromoSection/ClassicPromo.tsx (1)
ClassicPromo(7-38)packages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsx (1)
ImmersivePromo(13-56)packages/visual-editor/src/components/pageSections/PromoSection/SpotlightPromo.tsx (1)
SpotlightPromo(13-59)packages/visual-editor/src/components/pageSections/PromoSection/CompactPromo.tsx (1)
CompactPromo(7-70)packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx (1)
PromoEmptyState(5-53)
packages/visual-editor/src/components/pageSections/PromoSection/PromoMedia.tsx (2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (2)
PromoData(36-50)PromoStyles(52-89)packages/visual-editor/src/components/pageSections/index.ts (2)
PromoData(55-55)PromoStyles(56-56)
packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx (1)
packages/visual-editor/src/internal/hooks/useMessageReceivers.ts (1)
useTemplateMetadata(169-169)
🪛 Biome (2.1.2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
[error] 430-432: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 445-452: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx
[error] 15-15: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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 (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
- GitHub Check: create-dev-release
🔇 Additional comments (6)
packages/visual-editor/locales/sk/visual-editor.json (3)
197-197: LGTM!The translation "Zarovnanie kontajnera" is accurate Slovak for "Container alignment".
300-300: LGTM!Directional translations are correct: "Dole" (down/below) and "Hore" (up/above) are appropriate Slovak terms for bottom and top directions respectively.
Also applies to: 395-395
612-613: LGTM!The Slovak translations are now properly localized. "Propagačný banner" and "Propagačné médiá" are consistent and linguistically correct, using the Slovak adjective "propagačný" (promotional) rather than the English "promo". This addresses the previous review feedback about maintaining consistency with other locales.
packages/visual-editor/locales/ro/visual-editor.json (3)
196-196: LGTM!The Romanian translation "Alinierea containerului" (Container alignment) is grammatically correct and consistent with other alignment-related translations in the file.
299-299: LGTM! Previous issue resolved.The empty
bottom_directionvalue has been correctly set to "Fund", making it consistent with the"bottom": "Fund"translation on line 298.
611-611: LGTM!The Romanian translation "Banner promoțional" (Promotional banner) is appropriate and follows common usage in Romanian UI contexts.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/visual-editor/locales/tr/visual-editor.json (1)
197-197: LGTM! Turkish translations are complete and properly structured.All required localization keys are present with appropriate Turkish translations. Directional key pairs were previously reviewed.
Also applies to: 300-300, 395-395, 613-613
packages/visual-editor/locales/pl/visual-editor.json (1)
198-198: LGTM! Polish translations are properly localized.All required keys are present with appropriate Polish translations:
containerAlignment(Wyrównanie kontenera),bottom_direction(Dół),top_direction(Góra), andpromoBanner(Baner promocyjny).Also applies to: 301-301, 396-396, 614-614
🧹 Nitpick comments (2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx (2)
2497-2497: Improve type safety for the test helper parameter.The
anytype reduces type safety and could mask issues if the props structure changes.🔎 Proposed fix for type safety
-const isBrandColorTest = (props: any) => { +const isBrandColorTest = (props: { styles?: { backgroundColor?: { bgColor?: string } } }) => { const bg = props?.styles?.backgroundColor?.bgColor; return typeof bg === "string" && BRAND_COLOR_BG_ALLOWLIST.has(bg); };
2559-2561: Consider extracting the video delay to a named constant.The hard-coded 1000ms delay is a magic number that could benefit from documentation.
🔎 Proposed refactor
Add near the top of the file (after imports):
+// Allow time for video thumbnails and embeds to load before screenshot +const VIDEO_LOAD_DELAY_MS = 1000; + const promoData = {Then update the delay:
if (props?.data?.media === "video") { - await delay(1000); + await delay(VIDEO_LOAD_DELAY_MS); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (58)
packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [classic] version 50 props with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [classic] version 50 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [classic] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [compact] version 50 with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [compact] version 50 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [compact] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [immersive] version 50 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [immersive] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [spotlight] version 50 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] [spotlight] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] version 0 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[desktop] 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/PromoSection/[desktop] version 30 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [classic] version 50 props with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [classic] version 50 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [classic] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [compact] version 50 with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [compact] version 50 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [compact] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [immersive] version 50 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [immersive] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [spotlight] version 50 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] [spotlight] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] default props with empty document.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] version 0 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] version 0 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] 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/PromoSection/[mobile] version 16 props with missing ctaType.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] 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/PromoSection/[mobile] version 24 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] version 30 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] version 30 props with mixed values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] version 48 props with translatable image and CTA.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[mobile] version 5 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [classic] version 50 props with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [classic] version 50 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [classic] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [compact] version 50 with constant values and image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [compact] version 50 with constant values and video.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [compact] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [immersive] version 50 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [immersive] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [spotlight] version 50 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] [spotlight] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] default props with empty document.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] version 0 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] version 0 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[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/PromoSection/[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/PromoSection/[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/PromoSection/[tablet] version 24 with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] version 30 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] version 30 props with mixed values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] version 48 props with translatable image and CTA.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[tablet] version 5 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PromoSection/[xl] [compact] version 50 with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (31)
packages/visual-editor/locales/cs/visual-editor.jsonpackages/visual-editor/locales/da/visual-editor.jsonpackages/visual-editor/locales/de/visual-editor.jsonpackages/visual-editor/locales/en-GB/visual-editor.jsonpackages/visual-editor/locales/en/visual-editor.jsonpackages/visual-editor/locales/es/visual-editor.jsonpackages/visual-editor/locales/et/visual-editor.jsonpackages/visual-editor/locales/fi/visual-editor.jsonpackages/visual-editor/locales/fr/visual-editor.jsonpackages/visual-editor/locales/hr/visual-editor.jsonpackages/visual-editor/locales/hu/visual-editor.jsonpackages/visual-editor/locales/it/visual-editor.jsonpackages/visual-editor/locales/ja/visual-editor.jsonpackages/visual-editor/locales/lt/visual-editor.jsonpackages/visual-editor/locales/lv/visual-editor.jsonpackages/visual-editor/locales/nb/visual-editor.jsonpackages/visual-editor/locales/nl/visual-editor.jsonpackages/visual-editor/locales/pl/visual-editor.jsonpackages/visual-editor/locales/pt/visual-editor.jsonpackages/visual-editor/locales/ro/visual-editor.jsonpackages/visual-editor/locales/sk/visual-editor.jsonpackages/visual-editor/locales/sv/visual-editor.jsonpackages/visual-editor/locales/tr/visual-editor.jsonpackages/visual-editor/locales/zh-TW/visual-editor.jsonpackages/visual-editor/locales/zh/visual-editor.jsonpackages/visual-editor/src/components/migrations/0050_promo_variants.tspackages/visual-editor/src/components/migrations/migrationRegistry.tspackages/visual-editor/src/components/pageSections/HeroSection.test.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsxpackages/visual-editor/src/docs/ai/components.d.tspackages/visual-editor/src/docs/components.md
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/lt/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/src/components/pageSections/HeroSection.test.tsx
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/fr/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/es/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/hr/visual-editor.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-12T20:17:37.033Z
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:37.033Z
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.tspackages/visual-editor/src/components/migrations/0050_promo_variants.ts
📚 Learning: 2025-12-23T16:36:32.810Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/locales/ro/visual-editor.json:394-394
Timestamp: 2025-12-23T16:36:32.810Z
Learning: In Romanian locale files (ro), "Top" is an acceptable translation for directional terms like "top_direction" rather than the native Romanian "Sus". The maintainer benlife5 confirmed this is the preferred translation in packages/visual-editor/locales/ro/visual-editor.json.
Applied to files:
packages/visual-editor/locales/ro/visual-editor.jsonpackages/visual-editor/locales/en-GB/visual-editor.jsonpackages/visual-editor/locales/en/visual-editor.jsonpackages/visual-editor/locales/zh-TW/visual-editor.jsonpackages/visual-editor/locales/da/visual-editor.jsonpackages/visual-editor/locales/pl/visual-editor.jsonpackages/visual-editor/locales/et/visual-editor.jsonpackages/visual-editor/locales/nl/visual-editor.jsonpackages/visual-editor/locales/tr/visual-editor.jsonpackages/visual-editor/locales/sk/visual-editor.jsonpackages/visual-editor/locales/pt/visual-editor.json
📚 Learning: 2025-12-23T16:30:37.478Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In CompactHero and CompactPromo components, duplicate IDs on image elements are intentional: conditional visibility classes (hidden/sm:block/sm:hidden) ensure only one image renders at a time based on responsive breakpoints and desktopImagePosition/mobileImagePosition settings.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsxpackages/visual-editor/src/components/migrations/0050_promo_variants.tspackages/visual-editor/src/docs/components.md
🧬 Code graph analysis (2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx (4)
packages/visual-editor/src/components/testing/componentTests.setup.ts (4)
ComponentTest(109-121)transformTests(92-107)delay(126-127)axe(77-81)packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (1)
PromoSection(291-652)packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
migrationRegistry(58-109)packages/visual-editor/src/components/categories/SlotsCategory.tsx (1)
SlotsCategoryComponents(190-263)
packages/visual-editor/src/docs/ai/components.d.ts (3)
packages/visual-editor/src/editor/YextEntityFieldSelector.tsx (1)
YextEntityField(53-70)packages/visual-editor/src/types/images.ts (1)
AssetImageType(57-63)packages/visual-editor/src/utils/themeConfigOptions.ts (1)
BackgroundStyle(36-43)
⏰ 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). (2)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (14)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx (2)
1-62: LGTM!The imports and test fixture are well-organized. The
promoDatafixture provides comprehensive test data with realistic promo information including multiple image thumbnails, which is good for testing responsive image handling.
64-2484: Comprehensive test coverage across migration versions and variants.The test suite thoroughly covers:
- Migration paths from versions 0 through 50
- All four promo variants (classic, immersive, spotlight, compact) introduced in this PR
- Both entity and constant values
- Image and video media types
- Various slot configurations and positioning options
The explicit test fixtures, while lengthy, make each test case self-contained and easier to debug. The
includeXLViewportflag on line 1992 is a good addition for testing responsive behavior at larger viewports.packages/visual-editor/locales/en/visual-editor.json (1)
196-196: LGTM! Localization keys align with PromoSection variant architecture.The additions of
containerAlignment,bottom_direction,top_direction, andpromoBannerare properly structured and provide clear English labels for the new variant-based PromoSection features described in the PR.Also applies to: 299-299, 394-394, 612-612
packages/visual-editor/locales/zh-TW/visual-editor.json (1)
197-197: LGTM! Traditional Chinese translations are properly localized.All new keys are translated appropriately:
containerAlignment(容器對齊),bottom_direction(底部),top_direction(頂部), andpromoBanner(促銷橫幅).Also applies to: 300-300, 396-396, 614-614
packages/visual-editor/locales/en-GB/visual-editor.json (1)
196-196: LGTM! British English localization matches standard conventions.All new keys are properly added with translations consistent with British English conventions.
Also applies to: 299-299, 394-394, 612-612
packages/visual-editor/locales/da/visual-editor.json (1)
203-203: LGTM! Danish translations are complete and properly structured.All new localization keys are present with appropriate Danish translations:
containerAlignment(Beholderjustering),bottom_direction(Nederst),top_direction(Top), andpromoBanner(Promo banner).Also applies to: 307-307, 402-402, 620-620
packages/visual-editor/locales/nl/visual-editor.json (1)
197-197: LGTM! Dutch translations are properly localized.All new keys are present with appropriate Dutch translations:
containerAlignment(Uitlijning van containers),bottom_direction(Onder),top_direction(Boven), andpromoBanner(Promobanner).Also applies to: 300-300, 395-395, 613-613
packages/visual-editor/locales/sk/visual-editor.json (1)
198-198: LGTM! Slovak translations are complete and properly localized.All new keys are present with appropriate Slovak translations. The
promoBannertranslation (Propagačný banner) properly uses Slovak language as previously requested, andpromoMediais consistently translated as Propagačné médiá.Also applies to: 301-301, 396-396, 614-615
packages/visual-editor/locales/pt/visual-editor.json (1)
197-197: LGTM!The new locale keys (
containerAlignment,bottom_direction,top_direction,promoBanner) are properly translated to Portuguese and align with the variant-based PromoSection architecture introduced in this PR.Also applies to: 300-300, 395-395, 613-613
packages/visual-editor/locales/et/visual-editor.json (1)
197-197: LGTM!The new locale keys are properly translated to Estonian and consistent with the broader localization updates across the PR.
Also applies to: 300-300, 395-395, 613-613
packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
51-51: LGTM!The
promoVariantsmigration is correctly imported and appended to the registry. The migration index (50) aligns with the file naming convention, and placement at the end ensures proper execution order.Also applies to: 108-108
packages/visual-editor/src/docs/components.md (1)
669-707: LGTM!The PromoSection documentation accurately reflects the new variant-driven architecture with:
- Updated screenshot path for the new variant-based rendering
- New
data.backgroundImagefield- New style props (
variant,desktopImagePosition,mobileImagePosition,containerAlignment,imageHeight) replacing the previousorientationfield- Clear descriptions and default values
packages/visual-editor/src/docs/ai/components.d.ts (1)
717-763: LGTM!The type definitions properly reflect the new variant-driven PromoSection architecture:
PromoDatanow includesbackgroundImageconsistent withHeroDataPromoStyleshas appropriate variant options and positioning fields with clear JSDoc defaults- Types align with the migration logic and documentation updates
packages/visual-editor/locales/ro/visual-editor.json (1)
197-197: LGTM!The Romanian locale updates are correct. Based on learnings, "Top" is confirmed as an acceptable translation for
top_directionin Romanian, and all other translations (containerAlignment,bottom_direction,promoBanner) are properly localized.Also applies to: 300-300, 395-395, 613-613
packages/visual-editor/src/components/migrations/0050_promo_variants.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.test.tsx
Show resolved
Hide resolved
...testing/screenshots/HeroSection/[tablet] [compact] version 31 props with constant values.png
Show resolved
Hide resolved
...components/testing/screenshots/PromoSection/[desktop] version 0 props with entity values.png
Show resolved
Hide resolved
...components/testing/screenshots/PromoSection/[mobile] version 0 props with constant value.png
Show resolved
Hide resolved
...rc/components/testing/screenshots/PromoSection/[tablet] default props with document data.png
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (2)
212-212: Fix build failure: ThemeOptions.VERTICAL_POSITION does not exist.Same issue as in HeroSection.tsx line 277. The pipeline fails because
ThemeOptions.VERTICAL_POSITIONis not defined. You need to either add this property toThemeOptionsor use inline options.🔎 Proposed fix using inline options (temporary)
mobileImagePosition: YextField( msg("fields.mobileImagePosition", "Mobile Image Position"), { type: "radio", - options: ThemeOptions.VERTICAL_POSITION, + options: [ + { + label: msg("fields.options.top", "Top", { + context: "bottom_direction", + }), + value: "top", + }, + { + label: msg("fields.options.bottom", "Bottom", { + context: "bottom_direction", + }), + value: "bottom", + }, + ], } ),
402-441: Add explicit fallthrough annotations to silence linter errors.The switch statement uses intentional fallthrough (compact → classic at lines 403-405, and immersive → spotlight at lines 418-425). Add
/* falls through */comments after the fallthrough cases to satisfy the linter and make the intent explicit for future maintainers.🔎 Proposed fix
switch (data.props.styles.variant) { case "compact": { // compact should remove the same fields removed by classic } + /* falls through */ case "classic": { fields = updateFields( fields, [ "data.objectFields.backgroundImage", "styles.objectFields.imageHeight", ], undefined ); break; } case "immersive": { fields = updateFields( fields, ["slots.ImageSlot", "styles.objectFields.backgroundColor"], undefined ); // immersive should also remove the fields removed by spotlight } + /* falls through */ case "spotlight": { fields = updateFields( fields, [ "styles.objectFields.mobileImagePosition", "styles.objectFields.desktopImagePosition", "data.objectFields.media", data.props.data.promo.constantValueEnabled ? undefined : "data.objectFields.backgroundImage", ], undefined ); break; } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/components/pageSections/HeroSection.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In CompactHero and CompactPromo components, duplicate IDs on image elements are intentional: conditional visibility classes (hidden/sm:block/sm:hidden) ensure only one image renders at a time based on responsive breakpoints and desktopImagePosition/mobileImagePosition settings.
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In CompactHero and CompactPromo components, the 2xl breakpoint applies unconditional left padding because once the container reaches max-width (1440px), the design requires centering with consistent left padding regardless of desktopImagePosition.
📚 Learning: 2025-12-15T20:59:25.571Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 943
File: packages/visual-editor/src/components/contentBlocks/image/Image.tsx:175-191
Timestamp: 2025-12-15T20:59:25.571Z
Learning: In the ImageWrapper component at packages/visual-editor/src/components/contentBlocks/image/Image.tsx, when an Image is wrapped in a MaybeLink, the aria-label should be omitted because the image's alt text serves as the accessible name for the link. Adding an aria-label would override the image's alt text.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-12-12T20:17:37.033Z
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:37.033Z
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/pageSections/PromoSection/PromoSection.tsx
📚 Learning: 2025-12-23T16:30:37.478Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In CompactHero and CompactPromo components, duplicate IDs on image elements are intentional: conditional visibility classes (hidden/sm:block/sm:hidden) ensure only one image renders at a time based on responsive breakpoints and desktopImagePosition/mobileImagePosition settings.
Applied to files:
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
📚 Learning: 2025-12-23T16:30:37.478Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In hero variant components (e.g., CompactHero.tsx and CompactPromo.tsx), duplicate IDs on image elements may be intentional because visibility is controlled with responsive utility classes (hidden/sm:block/sm:hidden), ensuring only one image is rendered at a time based on breakpoints and desktopImagePosition/mobileImagePosition settings. When reviewing similar files, treat duplicate IDs as acceptable only if these visibility rules are clearly documented in code or accompanying comments, and verify accessibility (alt text, aria-labels) and that unique semantics are preserved where required. If a single element is conditionally rendered, prefer stable IDs or remove duplicates if not needed.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-12-23T16:30:37.478Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: Enforce that at the 2xl breakpoint (max-width: 1440px), CompactHero and related hero variant components apply a consistent left padding, making the container centered with the same left padding regardless of desktopImagePosition. Do not conditionally vary left padding based on image position; ensure styles/components reflect a fixed left padding to maintain symmetrical centering. Apply this guideline to CompactHero and CompactPromo (and other heroVariants in the same directory if they share the same layout rules); update CSS/inline styles accordingly and adjust tests to verify padding behavior at 2xl.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
📚 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/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
🧬 Code graph analysis (1)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (7)
packages/visual-editor/src/types/images.ts (1)
AssetImageType(57-63)packages/visual-editor/src/components/pageSections/HeroSection.tsx (1)
updateFields(660-692)packages/visual-editor/src/components/pageSections/PromoSection/ClassicPromo.tsx (1)
ClassicPromo(7-38)packages/visual-editor/src/components/pageSections/PromoSection/ImmersivePromo.tsx (1)
ImmersivePromo(13-56)packages/visual-editor/src/components/pageSections/PromoSection/SpotlightPromo.tsx (1)
SpotlightPromo(13-59)packages/visual-editor/src/components/pageSections/PromoSection/CompactPromo.tsx (1)
CompactPromo(7-70)packages/visual-editor/src/components/pageSections/PromoSection/PromoEmptyState.tsx (1)
PromoEmptyState(5-53)
🪛 Biome (2.1.2)
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
[error] 403-405: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 418-425: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
🪛 GitHub Actions: Create Dev Release
packages/visual-editor/src/components/pageSections/HeroSection.tsx
[error] 277-277: TypeScript error TS2339: Property 'VERTICAL_POSITION' does not exist on type '{ HEADING_LEVEL: { label: string; value: HeadingLevel; }[]; TEXT_TRANSFORM: { label: MsgString; value: string; }[]; LETTER_SPACING: { label: string; value: string; }[]; BACKGROUND_COLOR: ComboboxOptionGroup[]; ... 12 more ...; MAX_WIDTH: { ...; }[]; }'. Command: pnpm run build
🪛 GitHub Actions: main
packages/visual-editor/src/components/pageSections/HeroSection.tsx
[error] 277-277: TS2339: Property 'VERTICAL_POSITION' does not exist on type '{ HEADING_LEVEL: { label: string; value: HeadingLevel; }[]; TEXT_TRANSFORM: { label: MsgString; value: string; }[]; LETTER_SPACING: { label: string; value: string; }[]; BACKGROUND_COLOR: ComboboxOptionGroup[]; ... 12 more ...; MAX_WIDTH: { ...; }[]; }'.
⏰ 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). (1)
- GitHub Check: semgrep/ci
🔇 Additional comments (8)
packages/visual-editor/src/components/pageSections/HeroSection.tsx (2)
533-533: Image width behavior updated for compact variant.The className change from
"w-full sm:w-fit h-full"to"w-full h-full"removes responsive width fitting on small screens. This aligns with the layout refactor in CompactHero.tsx where images now usesm:w-[50%]. Ensure this produces the intended visual result across breakpoints.
662-670: LGTM! Defensive improvement to updateFields.The signature change to
(string | undefined)[]with a guard to skip falsy paths is a good defensive programming practice. This allows callers to conditionally passundefinedin the paths array (as seen in PromoSection.tsx line 433-435) without breaking the function.packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx (2)
34-92: LGTM! Well-structured responsive layout refactor.The restructured layout with separate mobile and desktop containers is clean and follows good responsive design patterns. The conditional padding and visibility classes appropriately handle the different image positions across breakpoints.
Based on learnings: The duplicate IDs on image elements (lines 46 and 82) are intentional since visibility classes ensure only one renders at a time.
17-17: Image width updated for responsive layout.The change to
"sm:w-[50%]"sets the image to 50% width on small screens and up, which aligns with the new two-column desktop layout structure. This works well with the conditionalw-2/4on the desktop content container (line 73).packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx (4)
451-481: LGTM! Variant-specific slot configuration.The variant-specific className updates for ImageSlot and VideoSlot are well-structured and properly scoped to each variant. The alignment application to DescriptionSlot is also appropriately handled.
504-510: LGTM! Background image field construction handles empty strings correctly.The ternary conditional properly handles empty field values. When
data.props?.data?.promo.fieldis empty, it returns an empty string instead of".image". This was correctly addressed per past review comments.Also applies to: 558-564
573-624: LGTM! Clean variant-based rendering with empty state handling.The render function is well-structured with proper variant selection and empty state handling for mapped fields in editor mode. The logic appropriately checks if a mapped entity field is empty and displays the
PromoEmptyStatecomponent to inform the user.
219-219: No action required.ThemeOptions.ALIGNMENTis properly defined inpackages/visual-editor/src/utils/themeConfigOptions.ts(line 430) and is being used correctly in this file.
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx
Show resolved
Hide resolved
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Created starter branch and new site with entity page group, DM, and locator. Navigated to the page source of the live site for each page group and verified that only the main page group page had the BCF. Test account: https://dev.yext.com/s/1000163760/yextsites/66475/pagesets --------- Co-authored-by: coltondemetriou <coltondemetriou@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/visual-editor/src/docs/components.md (2)
681-685: Minor: Inconsistent formatting in default values.The default values have inconsistent formatting:
- Line 683: Uses a period at the end (
"Placeholder image.")- Line 685: Uses single quotes around the value (
'image')Consider standardizing the format across all default values in the documentation for consistency.
696-696: Clarify the imageHeight description.The description "Image Height for the promo image with Immersive or Spotlight variant Minimum height: content height + Page Section Top/Bottom Padding 500px" is unclear:
- Missing punctuation/proper sentence structure
- The relationship of "500px" to the formula is ambiguous - is 500px the minimum value, or is it part of the calculation?
Consider revising to something like:
- "Minimum image height for immersive or spotlight variants. Must be at least 500px or content height + padding, whichever is greater."
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/docs/ai/components.d.tspackages/visual-editor/src/docs/components.mdpackages/visual-editor/src/utils/themeConfigOptions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/docs/ai/components.d.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:55.497Z
Learning: In CompactHero and CompactPromo components, duplicate IDs on image elements are intentional: conditional visibility classes (hidden/sm:block/sm:hidden) ensure only one image renders at a time based on responsive breakpoints and desktopImagePosition/mobileImagePosition settings.
📚 Learning: 2025-12-23T16:30:55.497Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:55.497Z
Learning: In CompactHero and CompactPromo components, duplicate IDs on image elements are intentional: conditional visibility classes (hidden/sm:block/sm:hidden) ensure only one image renders at a time based on responsive breakpoints and desktopImagePosition/mobileImagePosition settings.
Applied to files:
packages/visual-editor/src/docs/components.md
🧬 Code graph analysis (1)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
packages/visual-editor/src/utils/i18n/platform.ts (1)
msg(104-104)packages/visual-editor/src/utils/i18n/index.ts (1)
msg(5-5)
⏰ 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). (5)
- GitHub Check: call_unit_test / unit_tests (22.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (24.x)
- GitHub Check: create-dev-release
- GitHub Check: semgrep/ci
🔇 Additional comments (3)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
352-361: LGTM! Clean implementation of vertical positioning options.The new
verticalPositionOptionsfollows the established pattern for option definitions and correctly uses i18n with appropriate context.
442-442: LGTM! ThemeOptions export properly updated.The new
VERTICAL_POSITIONexport integrates cleanly with the existing ThemeOptions structure.packages/visual-editor/src/docs/components.md (1)
691-698: LGTM! Comprehensive documentation of new variant props.The expanded style props table clearly documents the new variant-driven architecture with appropriate descriptions distinguishing which variants each prop applies to.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx (1)
40-50: Duplicate ID pattern is acceptable; consider enhancing comment.The duplicate
id={id + "-image"}on both image instances (lines 42 and 89) is intentional and valid because visibility classes (hidden,sm:block,sm:hidden) ensure only one image is rendered at any viewport.To aid future maintainers, consider expanding the comment to explicitly note this pattern:
{/* Desktop left image / Mobile top image - shares ID with right/bottom image; only one visible at a time */}Based on learnings, this pattern is acceptable for CompactHero and CompactPromo when visibility is properly controlled.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/visual-editor/src/components/testing/screenshots/HeroSection/[desktop] [compact] version 17 props using constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[desktop] [compact] version 31 props with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[desktop] [compact] version 31 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [compact] version 17 props using constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [compact] version 31 props with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [compact] version 31 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[xl] [compact] version 31 props with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-23T16:30:37.478Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: In hero variant components (e.g., CompactHero.tsx and CompactPromo.tsx), duplicate IDs on image elements may be intentional because visibility is controlled with responsive utility classes (hidden/sm:block/sm:hidden), ensuring only one image is rendered at a time based on breakpoints and desktopImagePosition/mobileImagePosition settings. When reviewing similar files, treat duplicate IDs as acceptable only if these visibility rules are clearly documented in code or accompanying comments, and verify accessibility (alt text, aria-labels) and that unique semantics are preserved where required. If a single element is conditionally rendered, prefer stable IDs or remove duplicates if not needed.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-12-23T16:30:37.478Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx:38-41
Timestamp: 2025-12-23T16:30:37.478Z
Learning: Enforce that at the 2xl breakpoint (max-width: 1440px), CompactHero and related hero variant components apply a consistent left padding, making the container centered with the same left padding regardless of desktopImagePosition. Do not conditionally vary left padding based on image position; ensure styles/components reflect a fixed left padding to maintain symmetrical centering. Apply this guideline to CompactHero and CompactPromo (and other heroVariants in the same directory if they share the same layout rules); update CSS/inline styles accordingly and adjust tests to verify padding behavior at 2xl.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-12-15T20:59:25.571Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 943
File: packages/visual-editor/src/components/contentBlocks/image/Image.tsx:175-191
Timestamp: 2025-12-15T20:59:25.571Z
Learning: In the ImageWrapper component at packages/visual-editor/src/components/contentBlocks/image/Image.tsx, when an Image is wrapped in a MaybeLink, the aria-label should be omitted because the image's alt text serves as the accessible name for the link. Adding an aria-label would override the image's alt text.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
📚 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/components/pageSections/heroVariants/CompactHero.tsx
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx
⏰ 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). (5)
- GitHub Check: call_unit_test / unit_tests (24.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (22.x)
- GitHub Check: semgrep/ci
- GitHub Check: create-dev-release
🔇 Additional comments (5)
packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx (5)
7-28: LGTM! Width adjustment supports responsive layout.The change from
"w-full"to"sm:w-[50%]"(line 17) correctly implements the responsive behavior where the image occupies 50% width on desktop viewports, aligning with the restructured hero layout.
34-39: LGTM! Clean background and container structure.The Background wrapper (line 34) and outer container with
max-w-[1440px](line 37) provide a solid foundation for the responsive hero layout. The flex direction switching (flex-col sm:flex-row) correctly handles mobile stacking.
52-60: LGTM! Mobile container properly isolated.The mobile content container (lines 53-60) is correctly hidden on desktop (
sm:hidden) and has appropriate width constraints and padding for mobile viewports.
87-97: Duplicate ID pattern is acceptable here as well.Similar to the first image instance, the duplicate
id={id + "-image"}(line 89) is valid because conditional visibility classes ensure only one image renders at a time. The visibility logic (hidden sm:blockwhen mobile image is top,sm:hiddenwhen desktop image is left) correctly controls rendering.
62-85: Conditional margins don't match documented design requirement for consistent centering.The desktop container uses conditional padding and margins that vary based on
desktopImagePosition(lines 68-82):
- When image is left: applies
sm:pl-8 lg:pl-16+marginRight- When image is right: applies
sm:pr-8 lg:pr-16+marginLeftAccording to documented learnings, the design should apply consistent left padding at the max-width breakpoint (1440px) regardless of image position. The current implementation diverges from this requirement by making padding and margins conditional on image placement. Clarify whether this conditional approach is intentional or if consistent left-side padding should be restored across both image position scenarios.
...esting/screenshots/HeroSection/[desktop] [compact] version 31 props with constant values.png
Show resolved
Hide resolved
|
iImmersive/spotlight hero images not rendering in platform for v1.0.4, as well as with the updates in this pr, TAL but should probably fix it in this pr as well |
Also made the compact hero changes described in https://yext.atlassian.net/browse/OPAQF-9
Removed the RTF text color from the promo data since we no longer allow you to change the text color in KG