Conversation
…nto empty-slots
commit: |
WalkthroughAdds show/hide visibility controls across the visual editor: 11 new locale keys added for many platforms, a migration (0059) to apply visibility defaults, a new syncParentStyles utility, new SHOW_HIDE theme option, and extensive updates to page sections (styles.showSectionHeading and per-wrapper styles). Card wrappers expose styles that inject parentStyles into card components; cards sync parentStyles into their resolved data. Tests and docs updated to cover version 59 scenarios. Sequence Diagram(s)sequenceDiagram
participant Editor as Editor UI
participant Section as Page Section<br/> (e.g., EventSection)
participant Wrapper as Cards Wrapper<br/> (e.g., EventCardsWrapper)
participant Card as Card Component<br/> (e.g., EventCard)
participant Slot as Child Slot<br/> (Image/Date/CTA)
Editor->>Section: Update styles (e.g., set showSectionHeading / showImage)
Section->>Wrapper: Pass styles to CardsWrapperSlot (styles.*)
Wrapper->>Card: Inject parentStyles into Card props
Card->>Card: resolveData -> syncParentStyles(params, updatedData, keys)
Card->>Slot: Conditional render based on parentStyles.showImage / showDateTime / ...
Slot-->>Card: Rendered or skipped
Card-->>Wrapper: Emit updated data (with parentStyles applied)
Wrapper-->>Section: Render cards with applied visibility
Section-->>Editor: Updated preview
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx`:
- Around line 390-398: The show flags (hasPrice, hasDescription, hasCTA, hasBrow
and the other showImage/showTitle flags around lines 410-435) incorrectly rely
on parentStyles being defined and therefore become falsy when parentStyles is
undefined; update each boolean expression to default to true by using the
nullish coalescing fallback on the parentStyles property (e.g., change
parentStyles?.showPrice to parentStyles?.showPrice ?? true) while keeping the
existing conditionalRender and puck.isEditing checks intact so the card remains
visible when styles are missing.
In `@packages/visual-editor/src/docs/components.md`:
- Around line 740-743: The four table rows for styles.showCTA,
styles.showDescription, styles.showHeading, and styles.showMedia have the
default value appended to the Description cell; update each Markdown table row
so the Description column contains only the sentence (e.g., "Whether to show the
CTA.") and the Default column contains `true` (move the trailing "true" into the
Default column) for each of the corresponding symbol names (styles.showCTA,
styles.showDescription, styles.showHeading, styles.showMedia).
🟡 Minor comments (21)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx-53-53 (1)
53-53:⚠️ Potential issue | 🟡 MinorMalformed image URLs contain markdown link artifact.
Lines 53 and 153 contain URLs with an embedded
](https://...fragment — this looks like a markdown link syntax that was accidentally pasted into the string literal. These won't resolve to valid images.// Line 53: "https://images.unsplash.com/...&fit=max](https://images.unsplash.com/...&fit=max" // Line 153: "https://images.unsplash.com/...&fit=max](https://images.unsplash.com/...&fit=max"Strip the
](https://...suffix so each URL ends at&fit=max.🔧 Proposed fix
- url: "https://images.unsplash.com/photo-1504548840739-580b10ae7715?ixlib=rb-4.1.0&q=85&fm=jpg&crop=entropy&cs=srgb&height=360&width=640&fit=max](https://images.unsplash.com/photo-1504548840739-580b10ae7715?ixlib=rb-4.1.0&q=85&fm=jpg&crop=entropy&cs=srgb&height=360&width=640&fit=max", + url: "https://images.unsplash.com/photo-1504548840739-580b10ae7715?ixlib=rb-4.1.0&q=85&fm=jpg&crop=entropy&cs=srgb&height=360&width=640&fit=max",- url: "https://images.unsplash.com/photo-1502252430442-aac78f397426?ixlib=rb-4.1.0&q=85&fm=jpg&crop=entropy&cs=srgb&height=360&width=640&fit=max](https://images.unsplash.com/photo-1502252430442-aac78f397426?ixlib=rb-4.1.0&q=85&fm=jpg&crop=entropy&cs=srgb&height=360&width=640&fit=max", + url: "https://images.unsplash.com/photo-1502252430442-aac78f397426?ixlib=rb-4.1.0&q=85&fm=jpg&crop=entropy&cs=srgb&height=360&width=640&fit=max",Also applies to: 153-153
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx-2489-2502 (1)
2489-2502:⚠️ Potential issue | 🟡 MinorDuplicate test case — exact copy of the preceding entry.
Lines 2489–2502 are identical to lines 2475–2488 (same name, props, and version). This appears to be a copy-paste accident. Remove one of them.
🔧 Proposed fix — remove the duplicate
}, - { - name: "[classic] version 59 with showBusinessName, showGeomodifier, showHoursStatus false", - document: testDocument, - props: { - ...version59Props, - styles: { - ...version59Props.styles, - showBusinessName: false, - showGeomodifier: false, - showHoursStatus: false, - }, - }, - version: 59, - }, { name: "[immersive] version 59 with showBusinessName, showGeomodifier, showHoursStatus false",packages/visual-editor/src/docs/components.md-520-522 (1)
520-522:⚠️ Potential issue | 🟡 MinorDocument the default value for
showSectionHeading.The
showSectionHeadingproperty is missing a default value in the documentation. Based on similar sections, this should likely default totrue.packages/visual-editor/src/docs/components.md-223-225 (1)
223-225:⚠️ Potential issue | 🟡 MinorDocument the default value for
showSectionHeading.The table doesn't specify a default value for the
showSectionHeadingproperty. Other sections (e.g., FAQSection, PhotoGallerySection) document this property with a default oftrue. For consistency and completeness, please add the default value to the table.packages/visual-editor/src/docs/components.md-659-661 (1)
659-661:⚠️ Potential issue | 🟡 MinorDocument the default value for
showSectionHeading.The
showSectionHeadingproperty is missing a default value. Please add it to maintain consistency with other sections.packages/visual-editor/src/docs/components.md-768-770 (1)
768-770:⚠️ Potential issue | 🟡 MinorDocument the default value for
showSectionHeading.The
showSectionHeadingproperty is missing a default value. Please ensure consistency across all component documentation.packages/visual-editor/src/docs/components.md-879-881 (1)
879-881:⚠️ Potential issue | 🟡 MinorDocument the default value for
showSectionHeading.The
showSectionHeadingproperty is missing a default value. For consistency with FAQSection and PhotoGallerySection, this should likely be documented astrue.packages/visual-editor/src/docs/components.md-443-448 (1)
443-448:⚠️ Potential issue | 🟡 MinorAdd descriptions for the new visibility properties.
Five new
show*properties (showBusinessName,showGeomodifier,showHoursStatus,showPrimaryCTA,showSecondaryCTA) are missing descriptions in the Description column. Consider adding brief descriptions like "Whether to show the business name" to help users understand what each property controls.packages/visual-editor/src/docs/components.md-577-579 (1)
577-579:⚠️ Potential issue | 🟡 MinorDocument the default value for
showSectionHeading.The
showSectionHeadingproperty lacks a default value. Please add it for documentation completeness.packages/visual-editor/src/docs/components.md-853-855 (1)
853-855:⚠️ Potential issue | 🟡 MinorDocument the default value for
showSectionHeading.The
showSectionHeadingproperty lacks a default value in the documentation.packages/visual-editor/locales/platform/zh/visual-editor.json-488-503 (1)
488-503:⚠️ Potential issue | 🟡 MinorMistranslated "Show" as entertainment/exhibition noun instead of "Display".
Four new keys use incorrect Chinese translations where "Show" was interpreted as a noun (演出 = performance, 展会 = exhibition) rather than the verb "display" (显示). This is inconsistent with existing keys in the same file (e.g.,
showAddress= "显示地址",showPhone= "显示电话").
- Line 488:
showDate— "演出日期" → should be "显示日期"- Line 497:
showHours— "演出时间" → should be "显示营业时间"- Line 502:
showMedia— "展会媒体" → should be "显示媒体"- Line 503:
showName— "演出名称" → should be "显示名称"Proposed fix
- "showDate": "演出日期", + "showDate": "显示日期",- "showHours": "演出时间", + "showHours": "显示营业时间",- "showMedia": "展会媒体", + "showMedia": "显示媒体",- "showName": "演出名称", + "showName": "显示名称",packages/visual-editor/locales/platform/ja/visual-editor.json-485-503 (1)
485-503:⚠️ Potential issue | 🟡 MinorSeveral Japanese translations misinterpret "Show" as a noun instead of a verb.
Three new keys appear to have machine-translation artifacts where "Show" was treated as a noun (TV show / screening) rather than the verb "display":
Key Current Issue Suggested showDate(Line 488)"表示日"Reads as "display date" (noun compound) "日付を表示"showHours(Line 497)"上映時間"Means "screening time" (cinema) "営業時間を表示"showName(Line 503)"番組名"Means "program/show name" (TV) "名前を表示"Other
show*keys in this file correctly follow the"〜を表示"pattern (e.g.,showCategory:"カテゴリを表示",showHeading:"見出しを表示").Suggested fixes
- "showDate": "表示日", + "showDate": "日付を表示",- "showHours": "上映時間", + "showHours": "営業時間を表示",- "showName": "番組名", + "showName": "名前を表示",packages/visual-editor/locales/platform/nb/visual-editor.json-497-497 (1)
497-497:⚠️ Potential issue | 🟡 MinorPossible mistranslation for
showHours.
"Visningstider"translates to "showtimes" (cinema/theater context) rather than "Show Hours" (as in display business hours). All othershow*keys in this file consistently use the"Vis ..."prefix pattern (e.g.,"Vis dato","Vis overskrift"). This should likely be"Vis timer"or"Vis åpningstider"to stay consistent.Suggested fix
- "showHours": "Visningstider", + "showHours": "Vis åpningstider",packages/visual-editor/src/components/pageSections/heroVariants/HeroContent.tsx-33-75 (1)
33-75:⚠️ Potential issue | 🟡 MinorAvoid rendering an empty header when no reviews exist.
If only
showAverageReviewis true andreviewCountis 0,showHeaderstill renders an empty<header>. Consider gating the average-review portion so the header only renders when there’s content.✅ Suggested fix
- const showHeader = - styles.showBusinessName || - styles.showGeomodifier || - showHours || - styles.showAverageReview; + const showAverageReview = styles.showAverageReview && reviewCount > 0; + const showHeader = + styles.showBusinessName || + styles.showGeomodifier || + showHours || + showAverageReview; ... - {reviewCount > 0 && styles.showAverageReview && ( + {showAverageReview && ( <ReviewStars averageRating={averageRating} reviewCount={reviewCount}packages/visual-editor/locales/platform/nl/visual-editor.json-485-511 (1)
485-511:⚠️ Potential issue | 🟡 MinorMinor inconsistency in Dutch verb forms across new entries.
The new entries mix "Toon" (imperative, e.g.
showHeading: "Toon kop"), "tonen" (infinitive, e.g.showCategory: "Categorie tonen"), and "weergeven" (e.g.showDate: "Datum weergeven"). The pre-existing entries predominantly use "Toon" (e.g.showAddress: "Toon adres",showCTA: "Toon CTA"). Consider aligning the new entries to "Toon …" for consistency.packages/visual-editor/locales/platform/fr/visual-editor.json-496-496 (1)
496-496:⚠️ Potential issue | 🟡 MinorIncorrect French translation for
showHours.
"Horaires du spectacle"translates to "Show times" (theatrical performance), not "Show Hours" (display the hours). All othershow*keys in this file consistently use the"Afficher..."pattern (e.g.,"Afficher la date","Afficher le nom").Suggested fix
- "showHours": "Horaires du spectacle", + "showHours": "Afficher les horaires",packages/visual-editor/src/components/migrations/0059_show_hide_options.ts-6-16 (1)
6-16:⚠️ Potential issue | 🟡 Minor
delete props.datamutates the input argument — consider a non-mutating approach.Line 8 directly mutates the
propsobject passed into the transformation. Even though the return value uses...props(which won't include the deleted key), the caller's reference topropsis permanently modified. If the migration framework or any downstream consumer retains a reference to the original props, the missingdatakey could cause unexpected behavior.Suggested non-mutating approach
propTransformation: (props) => { const showDetailsColumn = props.data?.showDetailsColumn; - delete props.data; + const { data, ...rest } = props; return { - ...props, + ...rest, styles: { - ...props.styles, + ...rest.styles, showDetailsColumn: showDetailsColumn ?? true, }, }; },packages/visual-editor/locales/platform/it/visual-editor.json-497-497 (1)
497-497:⚠️ Potential issue | 🟡 MinorMistranslation: "Orari dello spettacolo" means "Showtimes", not "Show hours".
All other
show*keys in this file consistently use "Mostra ..." (e.g.,showDate→ "Mostra data",showCategory→ "Mostra categoria"). This key incorrectly interprets "Show" as a noun (spectacle) instead of a verb (display). Should be "Mostra orari" for consistency.Proposed fix
- "showHours": "Orari dello spettacolo", + "showHours": "Mostra orari",packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCard.tsx-149-153 (1)
149-153:⚠️ Potential issue | 🟡 MinorDefault parentStyles to true when undefined.
As written, missingparentStyleswill hide name/date even in edit mode; using?? truepreserves legacy behavior and avoids hiding before sync.🔧 Suggested change
- const showContributorName = - parentStyles?.showName && - Boolean(conditionalRender?.contributorName || puck.isEditing); - const showContributionDate = - parentStyles?.showDate && - Boolean(conditionalRender?.contributionDate || puck.isEditing); + const showContributorName = + (parentStyles?.showName ?? true) && + Boolean(conditionalRender?.contributorName || puck.isEditing); + const showContributionDate = + (parentStyles?.showDate ?? true) && + Boolean(conditionalRender?.contributionDate || puck.isEditing);Also applies to: 206-211, 304-321
packages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsx-498-509 (1)
498-509:⚠️ Potential issue | 🟡 MinorGuard against undefined showMedia to preserve legacy behavior.
!showMediatreatsundefinedasfalse, which can hide media fields on older data. Prefer an explicit=== falsecheck.🔧 Suggested change
- if (!data.props.styles.showMedia) { + if (data.props.styles.showMedia === false) { fields = updateFields( fields, [ "data.objectFields.media", "data.objectFields.backgroundImage", "styles.objectFields.imageHeight", ], undefined ); }packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.test.tsx-441-630 (1)
441-630:⚠️ Potential issue | 🟡 MinorNew v59 cases won’t load real data due to name-based fetch logic.
Because the test harness only allows fetch when the name includes “multiple nearby locations,” these new cases will stub fetch and only render loading state—so the show/hide flags won’t be exercised. Consider renaming the cases (or adding an explicit flag) so they load real data.🔧 Suggested rename to align with existing fetch logic
- name: "version 59 with showSectionHeading, showHours false", + name: "version 59 with multiple nearby locations (showSectionHeading, showHours false)", ... - name: "version 59 with showPhone, showAddress false", + name: "version 59 with multiple nearby locations (showPhone, showAddress false)",
🧹 Nitpick comments (9)
packages/visual-editor/src/docs/components.md (1)
651-651: Screenshot filename contains test-specific details.The screenshot filename includes test configuration details like
version%2059%20with%20showSectionHeading,%20showImage%20false, which may not be appropriate for user-facing documentation. Consider using a more generic screenshot or removing the test-specific details from the filename.packages/visual-editor/src/utils/cardSlots/syncParentStyles.ts (2)
12-16: Consider adding minimal type constraints instead ofany.The function works correctly, but all three parameters are typed
any, which removes all compile-time safety for callers. Even a lightweight generic or structural type would help prevent misuse:export const syncParentStyles = ( params: { parent?: { props: { styles?: Record<string, unknown> } } }, updatedData: { props: { parentStyles?: Record<string, unknown> } }, keys: string[] )This is optional given the generic nature of Puck's data model, but it would catch missing
propsat compile time rather than at runtime.
17-18: Defensive access onupdatedData.propsfor consistency.Line 17 uses optional chaining (
params.parent?.props.styles), but line 18 accessesupdatedData.props.parentStyleswithout it. IfupdatedData.propswere ever nullish, this would throw a TypeError. Consider adding optional chaining for consistency:- const currentStyles = updatedData.props.parentStyles || {}; + const currentStyles = updatedData.props?.parentStyles || {};Given that
updatedDatacomes from Puck'sresolveData,propsshould always exist, so this is a minor defensive coding suggestion.packages/visual-editor/src/components/pageSections/EventSection/EventCard.tsx (1)
291-302: Visibility defaults to hidden whenparentStylesis undefined.If
parentStylesisundefined(e.g. ifEventCardis ever rendered without anEventSectionparent, or beforeresolveDataruns), the&&short-circuits and all gated slots (image, dateTime, description, CTA) are hidden. This is fine for the current parent-child architecture, but it may be worth noting as a design decision — or adding a safe default (parentStyles?.showImage !== false) if standalone use ever becomes a possibility.packages/visual-editor/src/components/pageSections/ReviewsSection/ReviewsSection.tsx (1)
210-212: Minor inconsistency: optional chaining on a non-optional prop.
styles?.showSectionHeadinguses optional chaining here, butshowSectionHeadingis a requiredbooleanonReviewsSectionProps.styles(Line 67). Compare withEventSection.tsxLine 111 which usesstyles.showSectionHeadingwithout optional chaining. This is harmless but inconsistent across the PR.Suggested fix for consistency
- {styles?.showSectionHeading && ( + {styles.showSectionHeading && (packages/visual-editor/src/docs/ai/components.d.ts (1)
987-1006: Minor:@defaultvs@defaultValueinconsistency in PromoStyles JSDoc.The four new PromoStyles flags use
@default true(Lines 989, 994, 999, 1004), while every other section in this file uses@defaultValue true. This is a cosmetic inconsistency but could confuse documentation parsers or AI consumers that key on@defaultValue.Suggested fix
/** * Whether to show the media content, either image or video. - * `@default` true + * `@defaultValue` true */ showMedia: boolean; /** * Whether to show the heading text. - * `@default` true + * `@defaultValue` true */ showHeading: boolean; /** * Whether to show the description text. - * `@default` true + * `@defaultValue` true */ showDescription: boolean; /** * Whether to show the CTA. - * `@default` true + * `@defaultValue` true */ showCTA: boolean;packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsx (1)
28-28: Consider extractingupdateFieldsinto a shared utility module.
updateFieldsis a generic field manipulation helper that's now imported fromHeroSection.tsxby multiple unrelated components (NearbyLocationsCardsWrapper, and likely others). Importing a utility from a specific component file creates an implicit coupling.Moving it to a shared location (e.g.,
utils/fieldHelpers.ts) would make the dependency graph cleaner and the utility easier to discover.#!/bin/bash # Verify how many files import updateFields from HeroSection rg -n "from.*HeroSection" --type=ts -g '!*.test.*' -g '!*.d.ts'packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCard.tsx (1)
27-27: Limit syncParentStyles to keys used by this card.
showHeading/showIconaren’t part ofTestimonialCardProps.parentStyles, so trimming the list keeps saved data tidy.♻️ Suggested change
- updatedData = syncParentStyles(params, updatedData, [ - "showName", - "showDate", - "showHeading", - "showIcon", - ]); + updatedData = syncParentStyles(params, updatedData, [ + "showName", + "showDate", + ]);Also applies to: 392-397
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx (1)
237-247:imageConstrainis declared inparentStylesbut never synced or used.The
imageConstrainproperty is marked as required in the type, yet it is not included in thesyncParentStyleskeys (lines 542-550) and is no longer referenced in the rendering logic (line 412 now uses a static"w-full"). This leaves a type violation at runtime (property will beundefineddespite being required) and an unused import ofProductSectionImageConstrain.Consider removing
imageConstrainfrom the type and dropping theProductSectionImageConstrainimport.♻️ Proposed cleanup
parentStyles?: { variant: ProductSectionVariant; - imageConstrain: ProductSectionImageConstrain; showImage: boolean; showBrow: boolean; showTitle: boolean; showPrice: boolean; showDescription: boolean; showCTA: boolean; };-import { - ProductSectionImageConstrain, - ProductSectionVariant, -} from "./ProductSection.tsx"; +import { ProductSectionVariant } from "./ProductSection.tsx";
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
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/PromoSection/PromoSection.tsx (1)
480-509:⚠️ Potential issue | 🟡 Minor
showMedia=falsecan get "stuck" when switching to spotlight/immersive variants.For spotlight and immersive variants, the
showMediafield control is removed from the UI (line 487), meaning the user cannot toggle it. However, ifshowMediawas previously set tofalsewhile on a classic/compact variant, thatfalsevalue persists. The post-variant check at lines 498-509 then removes media-related fields, and per the AI summary,PromoMedia.tsxgates rendering onstyles.showMedia— so the background image won't render and the user has no way to re-enable it.Consider resetting
showMediatotrueinresolveDatawhen the variant is spotlight or immersive, so the stale value doesn't cause a broken state:Proposed fix in resolveData
resolveData: (data, params) => { let updatedData = { ...data }; + // For immersive/spotlight variants, media is always shown (as background) + if ( + data.props.styles.variant === "immersive" || + data.props.styles.variant === "spotlight" + ) { + updatedData = setDeep(updatedData, "props.styles.showMedia", true); + } // puck supports dot notation even though the type does not const mediaSubfield = "data.media" as any;
asanehisa
requested review from
benlife5,
briantstephan,
jwartofsky-yext and
mkilpatrick
There was a problem hiding this comment.
Should left context move up?
There was a problem hiding this comment.
Oh our non-image part of Hero in classic mode has "justify center" so it just centered
There was a problem hiding this comment.
Why is there a white floating box? Remove entirely if nothing in it?
There was a problem hiding this comment.
Floaty boi. Verify this is what we want.
4 participants
Will do:
Update Default Layout
Fix merge conflicts (with other PR's migrations)