Skip to content

Conversation

@rkeerthient
Copy link
Contributor

Ticket - OPAQF-60

  • Directory's title is by default coming from Name from KG instead of static [[name]]
  • Address in Directory card is migrated to slot.
    • Created a new prop hideCountry) on Address block to support hide country. This is defaulted to false.
  • Enabled Show Get Directions links on the Address slot (defaulted to false).
  • Added a new test case accordingly.
Screen.Recording.2026-02-09.at.5.53.03.PM.mov

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This 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)
Loading

Possibly related PRs

Suggested labels

create-dev-release

Suggested reviewers

  • mkilpatrick
  • benlife5
  • colton-demetriou
  • briantstephan
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: dynamic directory title and address converted to slots' directly and clearly describes the main changes: converting Directory title to use dynamic binding and Address to a slot system.
Description check ✅ Passed The description relates to the changeset by detailing how the Directory title now uses Knowledge Graph Name field, Address is migrated to a slot, Get Directions links are enabled, and a test case was added.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch OPAQF-60

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 | 🟡 Minor

Fix TitleSlot data shape to match HeadingTextSlot schema.
The Directory TitleSlot now places the binding directly under data, but other HeadingTextSlot instances (and the migration) use data.text. This can lead to an empty title in layouts that don’t run migrations. Recommend restoring the text wrapper.

🔧 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: hideCountry is not exposed in AddressStyleFields, making it non-configurable in the editor UI.

hideCountry is added to AddressProps["styles"] (Line 41) and correctly handled in AddressComponent (Lines 98, 145-152), but it's not included in AddressStyleFields (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 hideCountry field documenting that it's not exposed in the editor UI by design.

Also applies to: 95-152

benlife5
benlife5 previously approved these changes Feb 9, 2026
Copy link
Contributor

@asanehisa asanehisa left a 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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.

@benlife5 benlife5 merged commit a56d6f6 into main Feb 9, 2026
17 checks passed
@benlife5 benlife5 deleted the OPAQF-60 branch February 9, 2026 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants