Skip to content

Conversation

@k-gerner
Copy link
Contributor

Use display fields for the secondary CTA label/link

J=WAT-5225

locator_result_card_secondary_cta_display_field_demo.mov

@github-actions
Copy link
Contributor

Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Walkthrough

This PR splits EmbeddedFieldStringInput into two exports: EmbeddedFieldStringInputFromEntity (derives options from entity fields) and EmbeddedFieldStringInputFromOptions (accepts an options prop). TranslatableStringField gains an optional getOptions parameter and will render FromOptions when provided. CommandItemWithResolvedValue accepts an optional useOptionValue flag. LocatorResultCard.getDisplayFieldOptions now accepts string | string[], and several LocatorResultCard fields (including secondaryCTA.label and secondaryCTA.link) were converted to use TranslatableStringField with dynamic options. Barrel exports updated to expose the two new components.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant TranslatableStringField
    participant EmbeddedFieldStringInputFromOptions
    participant EmbeddedFieldStringInputFromEntity
    participant FieldSelector

    rect rgb(240,248,255)
    Note left of TranslatableStringField: Runtime branch based on getOptions presence
    end

    User->>TranslatableStringField: render
    alt getOptions provided
        TranslatableStringField->>EmbeddedFieldStringInputFromOptions: render with options list
        EmbeddedFieldStringInputFromOptions->>FieldSelector: present options (label/value, useOptionValueSublabel)
        FieldSelector-->>User: selectable options shown
    else no getOptions
        TranslatableStringField->>EmbeddedFieldStringInputFromEntity: render with entity-derived fields
        EmbeddedFieldStringInputFromEntity->>FieldSelector: present entity fields
        FieldSelector-->>User: selectable entity fields shown
    end

    User->>FieldSelector: select option
    FieldSelector-->>EmbeddedFieldStringInputFromOptions: return chosen value
    EmbeddedFieldStringInputFromOptions->>TranslatableStringField: onChange (debounced)
    Note right of FieldSelector: CommandItemWithResolvedValue may display option.value when useOptionValue is true
Loading

Possibly related PRs

Suggested labels

create-dev-release

Suggested reviewers

  • colton-demetriou
  • benlife5
  • mkilpatrick
  • jwartofsky-yext

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'feat: embed display fields in secondary cta dropdown' accurately describes the main change: enabling display fields for secondary CTA label/link fields through dynamic options.
Description check ✅ Passed The description 'Use display fields for the secondary CTA label/link' is directly related to the changeset, which refactors secondary CTA fields to use TranslatableString and dynamic options via getDisplayFieldOptions.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch embed-display-fields-in-secondary-cta-dropdown

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: 0

🧹 Nitpick comments (5)
packages/visual-editor/src/editor/index.ts (1)

23-26: Public API split into two embedded-field inputs; consider a temporary alias if consumers still use the old name

Exporting EmbeddedFieldStringInputFromEntity and EmbeddedFieldStringInputFromOptions is clear, but dropping EmbeddedFieldStringInput can be a breaking change for existing consumers of @yext/visual-editor. If this package is used externally, consider a deprecating alias:

export {
  EmbeddedFieldStringInputFromEntity,
  EmbeddedFieldStringInputFromOptions,
} from "./EmbeddedFieldStringInput.tsx";
+// Optional: temporary backwards-compatible alias for older callers.
+export {
+  EmbeddedFieldStringInputFromEntity as EmbeddedFieldStringInput,
+} from "./EmbeddedFieldStringInput.tsx";
packages/visual-editor/src/editor/TranslatableStringField.tsx (2)

17-33: Align JSDoc with the new getOptions parameter name

The doc comment still refers to @param options, but the function now takes getOptions?: () => DynamicOption<string>[]. Renaming the JSDoc param to getOptions will avoid confusion for callers reading the signature.


69-105: Handle DynamicOption entries with undefined value more explicitly

In the getOptions branch you map:

options={getOptions?.().map((opt) => ({
  label: opt.label,
  value: opt.value ?? "",
}))}

If a DynamicOption ever has value === undefined, this will produce an empty string, so:

  • The inserted token becomes [[ ]] when selected.
  • With useOptionValueSublabel={true}, the sublabel will be blank.

It would be safer to either filter such options out or choose a more meaningful fallback (e.g. use opt.label or throw/log in development) so invalid options don’t silently create unusable embedded tokens.

packages/visual-editor/src/components/LocatorResultCard.tsx (1)

516-529: Secondary CTA label/link now use TranslatableStringField; consider updating the prop types accordingly

Switching secondaryCTA.label and secondaryCTA.link to:

label: TranslatableStringField<any>(..., undefined, false, true, () =>
  getDisplayFieldOptions("type.string")
),
link: TranslatableStringField<any>(..., undefined, false, true, () =>
  getDisplayFieldOptions("type.string")
),

achieves the goal of letting users embed display fields into the CTA label and link. At render time you already pass both through resolveComponentData, which handles embedded tokens and translatable strings.

However, LocatorResultCardProps still types these as plain string:

secondaryCTA: {
  label: string;
  link: string;
  ...
};

For better type safety and self-documentation, consider updating these to reflect the actual shape (e.g. TranslatableString | string), and importing the TranslatableString type from your shared types module.

packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (1)

64-76: Options-based input and useOptionValue wiring look good; you can skip value resolution when only showing option.value

The new EmbeddedFieldStringInputFromOptions + useOptionValueSublabel flag, and the corresponding useOptionValue prop in CommandItemWithResolvedValue, cleanly support both:

  • Entity-driven options (preview resolved sample values).
  • Display-field/options-driven flows (show the option’s key instead).

Since the sublabel now sometimes uses only option.value:

{(resolvedValue || useOptionValue) && (
  <div className="ve-text-xs ...">
    {useOptionValue ? option.value : resolvedValue}
  </div>
)}

you can avoid unnecessary calls to resolveComponentData when useOptionValue is true, e.g.:

-  React.useEffect(() => {
-    if (isOpen && resolvedValue === undefined) {
+  React.useEffect(() => {
+    if (useOptionValue) {
+      return;
+    }
+    if (isOpen && resolvedValue === undefined) {
       ...
     }
-  }, [isOpen, option.value, streamDocument, resolvedValue]);
+  }, [isOpen, option.value, streamDocument, resolvedValue, useOptionValue]);

This keeps previews for entity-backed options while avoiding extra work for display-field-based ones.

Also applies to: 173-180, 241-244

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e019bc2 and 6cd4284.

📒 Files selected for processing (5)
  • packages/visual-editor/src/components/LocatorResultCard.tsx (2 hunks)
  • packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (6 hunks)
  • packages/visual-editor/src/editor/TranslatableStringField.tsx (2 hunks)
  • packages/visual-editor/src/editor/YextEntityFieldSelector.tsx (2 hunks)
  • packages/visual-editor/src/editor/index.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/editor/YextEntityFieldSelector.tsx
🧬 Code graph analysis (4)
packages/visual-editor/src/editor/TranslatableStringField.tsx (1)
packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (2)
  • EmbeddedFieldStringInputFromOptions (64-191)
  • EmbeddedFieldStringInputFromEntity (28-62)
packages/visual-editor/src/editor/YextEntityFieldSelector.tsx (2)
packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (1)
  • EmbeddedFieldStringInputFromEntity (28-62)
packages/visual-editor/src/editor/index.ts (1)
  • EmbeddedFieldStringInputFromEntity (24-24)
packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (2)
packages/visual-editor/src/editor/index.ts (2)
  • EmbeddedFieldStringInputFromEntity (24-24)
  • EmbeddedFieldStringInputFromOptions (25-25)
packages/visual-editor/src/editor/YextEntityFieldSelector.tsx (1)
  • getFieldsForSelector (396-416)
packages/visual-editor/src/components/LocatorResultCard.tsx (2)
packages/visual-editor/src/editor/DynamicOptionsSelector.tsx (1)
  • DynamicOption (33-36)
packages/visual-editor/src/editor/TranslatableStringField.tsx (1)
  • TranslatableStringField (25-121)
🔇 Additional comments (3)
packages/visual-editor/src/editor/YextEntityFieldSelector.tsx (1)

46-364: Swap to EmbeddedFieldStringInputFromEntity keeps behavior consistent while centralizing option logic

The new import and usage in ConstantValueInput correctly forward value, onChange, and filter, and keep showFieldSelector semantics. This reuses the shared getFieldsForSelector-based options logic without changing the constant-value flow for single string fields.

packages/visual-editor/src/components/LocatorResultCard.tsx (1)

237-248: getDisplayFieldOptions multi-type support looks correct

Allowing fieldTypeId: string | string[] and normalizing to fieldTypeIds before filtering keeps existing single-string callers working and cleanly supports future multi-type lookups:

const fieldTypeIds = Array.isArray(fieldTypeId) ? fieldTypeId : [fieldTypeId];
.filter((key) => fieldTypeIds.includes(displayFields![key].field_type_id))

No functional issues here.

packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (1)

28-61: Entity wrapper correctly reuses the new options-based input

EmbeddedFieldStringInputFromEntity now derives entityFieldOptions via getFieldsForSelector and delegates to EmbeddedFieldStringInputFromOptions. This keeps all cursor/embedding and debounce behavior in one place while preserving the existing entity-field semantics.

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/visual-editor/src/components/LocatorResultCard.tsx (2)

238-256: Rules of Hooks violation: getDisplayFieldOptions cannot call useTemplateMetadata hook.

getDisplayFieldOptions is a regular function that calls the useTemplateMetadata() hook on line 241. This violates React's Rules of Hooks, which state that hooks can only be called from React function components or custom hooks (functions starting with "use").

Even though getDisplayFieldOptions is invoked via the getOptions callback during rendering, it's still not a component or hook itself.

Solution: Convert getDisplayFieldOptions to a custom hook:

-const getDisplayFieldOptions = (
+const useDisplayFieldOptions = (
   fieldTypeId: string | string[]
 ): DynamicOption<string>[] => {
   const templateMetadata = useTemplateMetadata();

Then update all call sites to use useDisplayFieldOptions and call it at the component level (in the render context of a component that uses these fields), storing the result and passing it to getOptions:

// Example usage pattern - this would need to be adapted based on where fields are rendered
const stringFieldOptions = useDisplayFieldOptions("type.string");
const hoursFieldOptions = useDisplayFieldOptions("type.hours");
// etc.

// Then in field definitions:
getOptions: () => stringFieldOptions

Alternatively, if the metadata doesn't change during the component lifecycle, you could retrieve it once at a higher level and pass it down as a parameter to a pure getDisplayFieldOptions function.


158-168: Type inconsistency: secondaryCTA.label and secondaryCTA.link interface types don't match implementation.

The interface defines these fields as string (lines 161, 163), but:

  1. The field definitions use TranslatableStringField<any> (lines 517-530), which produces TranslatableString values
  2. The render code uses resolveComponentData() on these fields (lines 783-794), which expects TranslatableString type

Update the interface to match the implementation:

   secondaryCTA: {
     /** Label for the secondary CTA */
-    label: string;
+    label: TranslatableString;
     /** Template for the secondary CTA link, which can contain entity field references */
-    link: string;
+    link: TranslatableString;
     /** The variant for the secondary CTA */
     variant: CTAVariant;

Also ensure the import includes TranslatableString:

import { ..., TranslatableString, ... } from "@yext/visual-editor";
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6cd4284 and 5107ae4.

📒 Files selected for processing (1)
  • packages/visual-editor/src/components/LocatorResultCard.tsx (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T22:00:03.843Z
Learnt from: mkouzel-yext
Repo: yext/visual-editor PR: 833
File: packages/visual-editor/src/components/Locator.tsx:1050-1057
Timestamp: 2025-10-29T22:00:03.843Z
Learning: In packages/visual-editor/src/components/Locator.tsx, the AppliedFilters component is intentionally rendered in two locations (inside the filter modal and outside it) as per the design requirements. This dual rendering should not be flagged as a duplicate issue.

Applied to files:

  • packages/visual-editor/src/components/LocatorResultCard.tsx
🧬 Code graph analysis (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (3)
packages/visual-editor/src/editor/DynamicOptionsSelector.tsx (1)
  • DynamicOption (33-36)
packages/visual-editor/src/internal/hooks/useMessageReceivers.ts (1)
  • useTemplateMetadata (167-167)
packages/visual-editor/src/editor/TranslatableStringField.tsx (1)
  • TranslatableStringField (25-121)
🔇 Additional comments (2)
packages/visual-editor/src/components/LocatorResultCard.tsx (2)

246-248: LGTM: Array normalization and filtering logic is correct.

The implementation properly handles both single string and array inputs by normalizing to an array, then filtering display fields whose field_type_id matches any of the provided types.


524-530: The review comment is based on incorrect assumptions about available field types.

The locatorDisplayFields in templateMetadata only defines fields with types "type.string" and "type.phone". While "type.url" does exist elsewhere in the schema, it is not populated in locatorDisplayFields. The link field correctly uses getDisplayFieldOptions("type.string") because that is the only matching type available for field selection. Suggesting to filter on ["type.string", "type.url"] would have no effect since no URL-typed fields are defined in the locatorDisplayFields metadata that this component operates against.

Likely an incorrect or invalid review comment.

const fieldTypeIds = Array.isArray(fieldTypeId) ? fieldTypeId : [fieldTypeId];
return Object.keys(templateMetadata.locatorDisplayFields)
.filter((key) => displayFields[key].field_type_id === fieldTypeId)
.filter((key) => fieldTypeIds.includes(displayFields![key].field_type_id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this !? Ben had commented on it on my PR; it tells the TS compiler to not type check when it should.

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: 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/LocatorResultCard.tsx (1)

239-257: Remove the non-null assertion operator.

On Line 249, the non-null assertion operator (displayFields!) bypasses TypeScript's type checking. The guard on Line 243 already handles the null case, but TypeScript doesn't recognize this pattern. Based on learnings from past reviews, these operators should be avoided.

Consider restructuring to eliminate the need for the assertion:

 const getDisplayFieldOptions = (
   fieldTypeId: string | string[]
 ): DynamicOption<string>[] => {
   const templateMetadata = useTemplateMetadata();
-  if (!templateMetadata?.locatorDisplayFields) {
+  const displayFields = templateMetadata?.locatorDisplayFields;
+  if (!displayFields) {
     return [];
   }
-  const displayFields = templateMetadata.locatorDisplayFields;
   const fieldTypeIds = Array.isArray(fieldTypeId) ? fieldTypeId : [fieldTypeId];
   return Object.keys(templateMetadata.locatorDisplayFields)
-    .filter((key) => fieldTypeIds.includes(displayFields![key].field_type_id))
+    .filter((key) => fieldTypeIds.includes(displayFields[key].field_type_id))
     .map((key) => {
       const fieldData: FieldTypeData = displayFields[key];
       return {
         label: fieldData.field_name,
         value: key,
       };
     });
 };
♻️ Duplicate comments (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (1)

518-531: Replace TranslatableStringField<any> with proper typing.

The <any> type parameter bypasses TypeScript's type checking for both the label and link fields. This issue was previously flagged in past review comments and marked as addressed, but the code still uses <any>.

Apply the previously suggested fix:

-        label: TranslatableStringField<any>(
+        label: TranslatableStringField<TranslatableString>(
           msg("fields.label", "Label"),
           undefined,
           false,
           true,
           () => getDisplayFieldOptions("type.string")
         ),
-        link: TranslatableStringField<any>(
+        link: TranslatableStringField<TranslatableString>(
           msg("fields.link", "Link"),
           undefined,
           false,
           true,
           () => getDisplayFieldOptions("type.string")
         ),

Note: TranslatableString is already imported at Line 30.

🧹 Nitpick comments (1)
packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (1)

101-103: Optional: Simplify memoization.

The fieldOptions memoization simply returns the options prop. This memoization doesn't add value since options is already a stable reference from the parent.

Consider simplifying:

-  const fieldOptions = React.useMemo(() => {
-    return options;
-  }, [options]);
+  const fieldOptions = options;

Or remove the intermediate variable entirely and use options directly on Line 173.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5107ae4 and 5eb23bb.

📒 Files selected for processing (4)
  • packages/visual-editor/src/components/LocatorResultCard.tsx (4 hunks)
  • packages/visual-editor/src/docs/components.md (1 hunks)
  • packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (8 hunks)
  • packages/visual-editor/src/editor/TranslatableStringField.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/visual-editor/src/editor/TranslatableStringField.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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/docs/components.md
  • packages/visual-editor/src/components/LocatorResultCard.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/LocatorResultCard.tsx
🧬 Code graph analysis (2)
packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (2)
packages/visual-editor/src/editor/index.ts (2)
  • EmbeddedFieldStringInputFromEntity (24-24)
  • EmbeddedFieldStringInputFromOptions (25-25)
packages/visual-editor/src/editor/YextEntityFieldSelector.tsx (1)
  • getFieldsForSelector (396-416)
packages/visual-editor/src/components/LocatorResultCard.tsx (4)
packages/visual-editor/src/types/types.ts (1)
  • TranslatableString (205-205)
packages/visual-editor/src/editor/DynamicOptionsSelector.tsx (1)
  • DynamicOption (33-36)
packages/visual-editor/src/internal/hooks/useMessageReceivers.ts (1)
  • useTemplateMetadata (167-167)
packages/visual-editor/src/editor/TranslatableStringField.tsx (1)
  • TranslatableStringField (25-126)
⏰ 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). (3)
  • GitHub Check: call_unit_test / unit_tests (18.x)
  • GitHub Check: call_unit_test / unit_tests (20.x)
  • GitHub Check: semgrep/ci
🔇 Additional comments (5)
packages/visual-editor/src/components/LocatorResultCard.tsx (2)

30-30: LGTM!

The TranslatableString import is necessary for the updated type annotations in the LocatorResultCardProps interface.


162-164: Breaking change: CTA properties now support localization.

The label and link properties have been changed from string to TranslatableString, enabling locale-specific values for secondary CTA buttons. This is a breaking change to the public API that consumers will need to handle.

packages/visual-editor/src/docs/components.md (1)

444-444: LGTM!

The documentation correctly reflects the updated type signatures for resultCard.secondaryCTA.label and resultCard.secondaryCTA.link, now using TranslatableString instead of string.

packages/visual-editor/src/editor/EmbeddedFieldStringInput.tsx (2)

28-62: LGTM! Clean refactor using composition.

The refactoring of EmbeddedFieldStringInputFromEntity to compose EmbeddedFieldStringInputFromOptions is well-structured. The memoization correctly depends on entityFields and filter, and the separation of concerns improves maintainability.


193-252: LGTM! Correct implementation of conditional value display.

The useOptionValue prop correctly controls whether to display the raw option value or the resolved document value. The early return on Lines 212-214 efficiently skips unnecessary resolution when useOptionValue is true.

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: 0

🧹 Nitpick comments (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (1)

239-257: getDisplayFieldOptions array support looks good; consider tightening the param type

The normalization to fieldTypeIds and the includes filter over displayFields[key].field_type_id are correct and backward compatible with existing call sites.

If you want stronger typing, you could narrow the parameter to the actual field type id domain:

-const getDisplayFieldOptions = (
-  fieldTypeId: string | string[]
-): DynamicOption<string>[] => {
+const getDisplayFieldOptions = (
+  fieldTypeId: FieldTypeData["field_type_id"] | FieldTypeData["field_type_id"][]
+): DynamicOption<string>[] => {

and then reuse displayFields in Object.keys(displayFields) for slightly clearer intent.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5eb23bb and 2185fcf.

📒 Files selected for processing (1)
  • packages/visual-editor/src/components/LocatorResultCard.tsx (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/LocatorResultCard.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/LocatorResultCard.tsx
🧬 Code graph analysis (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (3)
packages/visual-editor/src/types/types.ts (1)
  • TranslatableString (205-205)
packages/visual-editor/src/editor/DynamicOptionsSelector.tsx (1)
  • DynamicOption (33-36)
packages/visual-editor/src/editor/TranslatableStringField.tsx (1)
  • TranslatableStringField (25-126)
⏰ 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). (3)
  • GitHub Check: call_unit_test / unit_tests (18.x)
  • GitHub Check: call_unit_test / unit_tests (20.x)
  • GitHub Check: semgrep/ci
🔇 Additional comments (2)
packages/visual-editor/src/components/LocatorResultCard.tsx (2)

159-169: Secondary CTA label/link type change to TranslatableString is consistent and safe

Props now use TranslatableString and are resolved via resolveComponentData at render time, with a sensible fallback for the label and default string values in DEFAULT_LOCATOR_RESULT_CARD_PROPS, which still satisfy the alias. This looks correct and keeps the runtime behavior coherent with the new translation support.

Also applies to: 772-800


514-531: Secondary CTA editor fields correctly wired to translatable + display field options

Using TranslatableStringField<TranslatableString> for both label and link with getDisplayFieldOptions("type.string") plugs the secondary CTA into the dynamic display field system and keeps the field value typed as TranslatableString. The argument ordering (label, undefined filter, false apply-all, true showFieldSelector, getOptions) matches the TranslatableStringField signature.

@mkouzel-yext mkouzel-yext merged commit 0ee97b2 into add-field-dropdowns Nov 21, 2025
15 checks passed
@mkouzel-yext mkouzel-yext deleted the embed-display-fields-in-secondary-cta-dropdown branch November 21, 2025 14:38
mkouzel-yext added a commit that referenced this pull request Nov 21, 2025
Adds all the new functionality to support customizable result cards in
the locator. This functionality was broken into #905 and #912 (which
also included #915), and now all the new features are jointly being
merged into main.

J=[PB-26919](https://yext.atlassian.net/browse/PB-26919)

---------

Co-authored-by: Kyle Gerner <kgerner@yext.com>
Co-authored-by: Kyle Gerner <49618240+k-gerner@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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