-
Notifications
You must be signed in to change notification settings - Fork 0
feat: dynamic directory title and address converted to slots #1036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds an AddressSlot to DirectoryCard and updates Directory to bind its TitleSlot to the "name" field (removing the default "[[name]]" constant). A new migration (0058) is registered to inject the TitleSlot and AddressSlot into existing Directory and DirectoryCard props when missing. defaultLayoutData and component defaultProps are updated to the new shape/version (58). A test case for the slotified address with showGetDirectionsLink was added. Sequence Diagram(s)sequenceDiagram
participant Editor as Editor (runtime)
participant MigrationRegistry as MigrationRegistry
participant Migration as 0058 Migration
participant Component as Directory / DirectoryCard props
participant Store as LayoutData / DefaultProps
Editor->>MigrationRegistry: request run migrations for layout
MigrationRegistry->>Migration: invoke fixDirectoryTitleBindingAndSlotifyAddress
Migration->>Component: transform props (add TitleSlot / AddressSlot when absent)
Migration->>Store: update defaultLayoutData version to 58
Component->>Editor: return migrated props
Editor->>Component: render Directory / DirectoryCard using slots (TitleSlot, AddressSlot)
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
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/vite-plugin/defaultLayoutData.ts (1)
4711-4718:⚠️ Potential issue | 🟡 MinorFix TitleSlot data shape to match HeadingTextSlot schema.
The Directory TitleSlot now places the binding directly underdata, but other HeadingTextSlot instances (and the migration) usedata.text. This can lead to an empty title in layouts that don’t run migrations. Recommend restoring thetextwrapper.🔧 Suggested fix
- data: { - constantValue: { en: "", hasLocalizedValue: "true" }, - constantValueEnabled: false, - field: "name", - }, + data: { + text: { + constantValue: { en: "", hasLocalizedValue: "true" }, + constantValueEnabled: false, + field: "name", + }, + },
🤖 Fix all issues with AI agents
In
`@packages/visual-editor/src/components/migrations/0057_dynamic_directory_title_binding_and_slotify_address.ts`:
- Around line 45-74: The Directory migration's propTransformation
unconditionally overwrites slots.TitleSlot; update propTransformation to first
check whether props.slots?.TitleSlot exists and if so return props unchanged
(preserving user customizations), otherwise create updatedProps with the new
TitleSlot; ensure you reference the Directory object and its propTransformation
and only mutate/return an updatedProps when TitleSlot is absent (mirroring the
guard used in DirectoryCard).
In `@packages/visual-editor/src/components/migrations/migrationRegistry.ts`:
- Line 58: The Directory branch in the migration
(fixDirectoryTitleBindingAndSlotifyAddress) unconditionally replaces TitleSlot
and may clobber user customizations; update that migration to perform the same
defensive check used by the DirectoryCard branch (e.g., inspect
props.slots?.TitleSlot and only proceed if there is no user-provided TitleSlot
or if the existing binding matches the old default pattern like "[[name]]"),
otherwise return props unchanged so existing custom TitleSlot bindings are
preserved.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/contentBlocks/Address.tsx (1)
41-41:hideCountryis not exposed inAddressStyleFields, making it non-configurable in the editor UI.
hideCountryis added toAddressProps["styles"](Line 41) and correctly handled inAddressComponent(Lines 98, 145-152), but it's not included inAddressStyleFields(Lines 61-80). This means users cannot toggle it from the editor — it can only be set via migration or programmatic defaults.If this is intentional (only for DirectoryCard's AddressSlot), consider adding a brief comment to the
hideCountryfield documenting that it's not exposed in the editor UI by design.Also applies to: 95-152
...editor/src/components/migrations/0058_dynamic_directory_title_binding_and_slotify_address.ts
Show resolved
Hide resolved
packages/visual-editor/src/components/migrations/migrationRegistry.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/contentBlocks/Address.tsx
Outdated
Show resolved
Hide resolved
asanehisa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update default layout data to use version 58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/vite-plugin/defaultLayoutData.ts`:
- Around line 4943-4949: The TitleSlot entry with id
"HeadingTextSlot-1a871989-a34d-426c-b24a-a1888c1a46ea" incorrectly places
constantValue, constantValueEnabled and field directly under data; move those
three properties under data.text (i.e., replace
data.{constantValue,constanValueEnabled,field} with
data.text.{constantValue,constantValueEnabled,field}) to match the structure
used by other HeadingTextSlot instances such as SiteNameSlot and the section
headings so the component can read the text value at runtime.
Ticket - OPAQF-60
Namefrom KG instead of static[[name]]Addressin Directory card is migrated to slot.hideCountry) on Address block to support hide country. This is defaulted tofalse.Show Get Directionslinks on the Address slot (defaulted tofalse).Screen.Recording.2026-02-09.at.5.53.03.PM.mov