feat: add color selection for CTAs #936
Conversation
|
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. |
WalkthroughAdds selectable site-color support for CTAs across the visual-editor package. Introduces an optional prop Sequence Diagram(s)sequenceDiagram
participant Editor as Editor UI (YextField)
participant Theme as ThemeOptions (SITE_COLOR)
participant Config as Component Config (CTAWrapper / CTAGroup / Address / GetDirections)
participant CTA as CTA component (atoms/cta.tsx)
participant Button as Rendered Button
Note over Editor,Theme: Author selects a site color in the editor
Editor->>Theme: request SITE_COLOR option groups
Theme-->>Editor: return siteColorOptions
Editor->>Config: save component data (styles.variant, styles.color, ctaType)
Config->>Config: resolveFields -> compute color field visibility (variant & ctaType)
Config->>CTA: render CTA with resolvedColor prop
CTA->>CTA: normalize token & compute dynamicStyle based on variant and color
CTA->>Button: render Button with computed inline style (unless presetImage)
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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: 15
🧹 Nitpick comments (2)
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
127-161: SITE_COLOR wiring and CTA variant relabeling look correct; consider de‑duplicating site color mappingThe new
siteColorOptionsandThemeOptions.SITE_COLORexport are consistent with howbackgroundColorOptionsis built, and the Solid/Outline relabeling for CTA variants keeps values stable ("primary","secondary"). All of this should work cleanly with the existingYextFieldhandling for"SITE_COLOR".You now have nearly identical logic for site colors in two places:
backgroundColorOptions’s"Site Colors"group- The new
siteColorOptionsIf you want to trim duplication, you could extract a small helper (e.g.
buildSiteColorOptions(backgroundColors)) and reuse it in both places, but this is non‑blocking.Also applies to: 163-178, 288-298, 405-413
packages/visual-editor/src/components/contentBlocks/Address.tsx (1)
2-7: Address Get Directions color selection is wired correctlyThe Address block’s new
color:
- Is surfaced via
AddressStyleFieldsas aSITE_COLORselect,- Defaults to
backgroundColors.color1.value,- Is resolved with a safe fallback, and
- Is only made visible in the editor for primary/secondary CTA variants via
resolveFields.Runtime usage (passing
colorinto theCTAwithctaType="getDirections") matches how the CTA component interprets colors, so behavior and config are consistent.Looks good.
If you later tighten typings around
AddressStyleFields, you could remove theas anycast onobjectFields, but that’s pre‑existing and not blocking.Also applies to: 23-41, 59-79, 81-92, 94-101, 148-160, 170-193, 194-201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/visual-editor/locales/cs/visual-editor.json(2 hunks)packages/visual-editor/locales/da/visual-editor.json(2 hunks)packages/visual-editor/locales/de/visual-editor.json(2 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(2 hunks)packages/visual-editor/locales/en/visual-editor.json(2 hunks)packages/visual-editor/locales/es/visual-editor.json(2 hunks)packages/visual-editor/locales/et/visual-editor.json(2 hunks)packages/visual-editor/locales/fi/visual-editor.json(2 hunks)packages/visual-editor/locales/fr/visual-editor.json(2 hunks)packages/visual-editor/locales/hr/visual-editor.json(2 hunks)packages/visual-editor/locales/hu/visual-editor.json(2 hunks)packages/visual-editor/locales/it/visual-editor.json(2 hunks)packages/visual-editor/locales/ja/visual-editor.json(2 hunks)packages/visual-editor/locales/lt/visual-editor.json(2 hunks)packages/visual-editor/locales/lv/visual-editor.json(2 hunks)packages/visual-editor/locales/nb/visual-editor.json(2 hunks)packages/visual-editor/locales/nl/visual-editor.json(2 hunks)packages/visual-editor/locales/pl/visual-editor.json(2 hunks)packages/visual-editor/locales/pt/visual-editor.json(2 hunks)packages/visual-editor/locales/ro/visual-editor.json(2 hunks)packages/visual-editor/locales/sk/visual-editor.json(2 hunks)packages/visual-editor/locales/sv/visual-editor.json(2 hunks)packages/visual-editor/locales/tr/visual-editor.json(2 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(2 hunks)packages/visual-editor/locales/zh/visual-editor.json(2 hunks)packages/visual-editor/src/components/atoms/cta.tsx(4 hunks)packages/visual-editor/src/components/contentBlocks/Address.tsx(8 hunks)packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx(6 hunks)packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx(7 hunks)packages/visual-editor/src/components/contentBlocks/GetDirections.tsx(3 hunks)packages/visual-editor/src/editor/YextField.tsx(1 hunks)packages/visual-editor/src/utils/themeConfigOptions.ts(3 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/components/contentBlocks/CtaWrapper.tsx
🧬 Code graph analysis (4)
packages/visual-editor/src/components/contentBlocks/GetDirections.tsx (5)
packages/visual-editor/src/components/atoms/cta.tsx (1)
CTAVariant(47-47)packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/utils/index.ts (2)
BackgroundStyle(33-33)backgroundColors(30-30)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-327)packages/visual-editor/src/editor/index.ts (1)
resolveDataFromParent(28-28)
packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx (2)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-327)
packages/visual-editor/src/components/contentBlocks/Address.tsx (3)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/utils/index.ts (2)
BackgroundStyle(33-33)backgroundColors(30-30)packages/visual-editor/src/editor/index.ts (1)
resolveDataFromParent(28-28)
packages/visual-editor/src/utils/themeConfigOptions.ts (4)
packages/visual-editor/src/internal/puck/ui/Combobox.tsx (1)
ComboboxOptionGroup(23-27)packages/visual-editor/src/utils/i18n/platform.ts (1)
msg(104-104)packages/visual-editor/src/utils/i18n/index.ts (1)
msg(5-5)packages/visual-editor/src/utils/index.ts (1)
backgroundColors(30-30)
🔇 Additional comments (16)
packages/visual-editor/locales/hu/visual-editor.json (1)
189-189: Approved: Color field label added.The Hungarian translation for the new "color" field label is correctly added.
packages/visual-editor/locales/zh-TW/visual-editor.json (1)
189-189: Empty translation values for solid/outline CTA variants.Lines 303-304 define
"solid": ""and"outline": ""with empty string values. Verify this is intentional (e.g., UI will display the English term as fallback) rather than placeholder translations that still need to be filled in.Also applies to: 303-304
packages/visual-editor/locales/de/visual-editor.json (1)
193-193: Consistent localization with complete translations.German locale provides full translations for new keys: "Farbe" (color), "Solide" (solid), and "Gliederung" (outline). Formatting and JSON structure are correct.
Also applies to: 307-309
packages/visual-editor/locales/hr/visual-editor.json (1)
189-189: Empty translation values follow a pattern across multiple locales.This file, along with zh-TW, fi, es, and others, leaves
"solid"and"outline"with empty string values. If this is intentional (fallback to English), ensure UI handles this gracefully. Otherwise, these are incomplete translations.Also applies to: 303-304
packages/visual-editor/locales/fi/visual-editor.json (1)
303-304: Incomplete translation for outline variant in Finnish locale.Line 304 shows
"outline": "Outline"without Finnish translation, while"solid": "Kiinteä"(line 303) is properly translated. This is inconsistent with other locales. Either translate "outline" to Finnish or use an empty string to match the pattern used in other incomplete locales.packages/visual-editor/locales/es/visual-editor.json (1)
302-303: Questionable Spanish translation for outline variant.Line 303:
"outline": "Describir"translates to "to describe" rather than a UI-appropriate term for the outline style. Consider using"Contorno"(outline/border) or"Esquema"(outline/scheme) instead, which better conveys the visual meaning in Spanish.packages/visual-editor/locales/en-GB/visual-editor.json (1)
188-188: Consistent and complete English localization.en-GB locale properly adds all required translation keys with appropriate English terminology: "Color", "Solid", and "Outline". JSON structure and formatting are correct.
Also applies to: 302-303
packages/visual-editor/locales/zh/visual-editor.json (1)
303-304: Consistent with empty-translation pattern observed in multiple locales.Like zh-TW, hr, and others, this file leaves solid and outline untranslated (empty strings). This pattern is cohesive; ensure it's by design.
packages/visual-editor/locales/da/visual-editor.json (1)
195-195: Complete and appropriate Danish localization.Danish locale properly translates all keys: "Farve" (color), "Solid" (kept in English, acceptable for design terminology), and "Disposition" (outline/layout). Formatting is consistent.
Also applies to: 310-311
packages/visual-editor/locales/en/visual-editor.json (1)
188-188: Translations complete for English locale. The "color" field and new CTA variant options ("solid", "outline") are properly translated and consistently placed. ✓Also applies to: 302-303
packages/visual-editor/locales/cs/visual-editor.json (1)
194-194: Translations complete for Czech locale. The "color" field and new CTA variant options are properly translated. ✓Also applies to: 310-311
packages/visual-editor/locales/fr/visual-editor.json (1)
188-188: LGTM - Complete translations provided.The French locale properly includes translations for all new fields (color, solid, outline).
Also applies to: 301-303
packages/visual-editor/locales/et/visual-editor.json (1)
189-189: LGTM - Complete translations provided.The Estonian locale includes proper translations for all new fields.
Also applies to: 302-304
packages/visual-editor/src/editor/YextField.tsx (1)
221-228: LGTM - Consistent with existing pattern.The SITE_COLOR handling mirrors the existing BACKGROUND_COLOR implementation and is correctly positioned before the hasSearch conditional.
packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx (1)
23-31: CTAGroup color wiring is consistent; dynamic show/hide can follow later when supportedThe new
colorprop and default (backgroundColors.color1.value) are correctly threaded from editor config intoCTA, andresolvedColorsafely falls back when older content lackscolor.Given CTA itself ignores color for
presetImagetypes, it’s acceptable that CTAGroup currently exposes color for those entries even though it has no effect, especially since you’ve already documented that CTAGroup can’t yet useresolveFieldsfor conditional visibility.No blocking issues here.
If you’d like to tighten the UX once CTAGroup supports
resolveFields, you can mirror the visibility logic used inCtaWrapper/Addressto hide the color field for non‑primary/secondary variants andpresetImageCTAs.Also applies to: 33-46, 52-57, 73-85, 105-107, 132-145
packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx (1)
33-47: CTAWrapper color support and conditional visibility look solid
styles.coloris correctly:
- Exposed as a
SITE_COLORselect,- Defaulted to
backgroundColors.color1.value,- Resolved with a safe fallback, and
- Passed through to
CTA.The
resolveFieldslogic that only shows the color selector for non‑presetImage CTAs with primary/secondary variants matches howCTAactually usescolor, so schema and runtime behavior are aligned.No changes needed here.
Also applies to: 67-115, 145-146, 159-173, 183-203, 205-227
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/visual-editor/locales/nb/visual-editor.json (1)
301-304: Previous translation issue has been addressed; consider verifying terminology choices.The previously flagged empty strings for CTA variant options have been resolved with Norwegian translations:
- "outline" → "Disposisjon"
- "solid" → "Fast"
These are reasonable translations. However, the prior review suggested alternative terminology ("Omriss"/"Kontur" for outline, "Fyll" for solid). While your choices are acceptable, consider verifying with Norwegian-speaking team members familiar with UI/design terminology to ensure the labels align with standard design conventions in your product.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
packages/visual-editor/locales/cs/visual-editor.json(2 hunks)packages/visual-editor/locales/da/visual-editor.json(2 hunks)packages/visual-editor/locales/de/visual-editor.json(2 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(2 hunks)packages/visual-editor/locales/en/visual-editor.json(2 hunks)packages/visual-editor/locales/es/visual-editor.json(2 hunks)packages/visual-editor/locales/et/visual-editor.json(2 hunks)packages/visual-editor/locales/fi/visual-editor.json(2 hunks)packages/visual-editor/locales/fr/visual-editor.json(2 hunks)packages/visual-editor/locales/hr/visual-editor.json(2 hunks)packages/visual-editor/locales/hu/visual-editor.json(2 hunks)packages/visual-editor/locales/it/visual-editor.json(2 hunks)packages/visual-editor/locales/ja/visual-editor.json(2 hunks)packages/visual-editor/locales/lt/visual-editor.json(2 hunks)packages/visual-editor/locales/lv/visual-editor.json(2 hunks)packages/visual-editor/locales/nb/visual-editor.json(2 hunks)packages/visual-editor/locales/nl/visual-editor.json(2 hunks)packages/visual-editor/locales/pl/visual-editor.json(2 hunks)packages/visual-editor/locales/pt/visual-editor.json(2 hunks)packages/visual-editor/locales/ro/visual-editor.json(2 hunks)packages/visual-editor/locales/sk/visual-editor.json(2 hunks)packages/visual-editor/locales/sv/visual-editor.json(2 hunks)packages/visual-editor/locales/tr/visual-editor.json(2 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(2 hunks)packages/visual-editor/locales/zh/visual-editor.json(2 hunks)packages/visual-editor/src/components/contentBlocks/GetDirections.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/visual-editor/locales/nl/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/fr/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/lt/visual-editor.json
- packages/visual-editor/locales/da/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
⏰ 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 (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (10)
packages/visual-editor/locales/it/visual-editor.json (1)
189-189: Previously flagged translation gaps now resolved with accurate Italian labels.The added "color" field and the updated CTA variant options ("outline" and "solid") now have proper Italian translations:
- "color": "Colore" (line 189)
- "outline": "Contorno" (line 301)
- "solid": "Solido" (line 304)
The translations are contextually accurate and consistent with standard Italian UI terminology. The previous review comment about empty translations has been successfully addressed.
Also applies to: 299-305
packages/visual-editor/locales/nb/visual-editor.json (1)
189-189: New color field translation is correct.Line 189 adds the "color" field with the label "Farge" (Color), which is an appropriate Norwegian translation.
packages/visual-editor/locales/de/visual-editor.json (1)
193-193: Locale translations are consistent and idiomatic. ✓The addition of the
colorfield and new CTA variant labels (outline,solid) are correctly translated into German and properly integrated alongside existing entries. JSON structure is valid.Also applies to: 306-310
packages/visual-editor/locales/en/visual-editor.json (1)
188-188: Locale translations are consistent and idiomatic. ✓The addition of the
colorfield and new CTA variant labels (outline,solid) are correctly integrated into the English locale. JSON structure and formatting are valid.Also applies to: 300-304
packages/visual-editor/locales/et/visual-editor.json (1)
189-189: Locale translations are consistent and idiomatic. ✓The addition of the
colorfield and new CTA variant labels (outline,solid) are correctly translated into Estonian and properly placed. JSON structure is valid.Also applies to: 301-305
packages/visual-editor/locales/es/visual-editor.json (1)
298-304: Verify display label alignment with PR changes.The PR notes indicate that CTA display labels are being renamed: "Primary" → "Solid" and "Secondary" → "Outline" respectively. However, the Spanish translations still show "Primario" (Primary) and "Secundario" (Secondary) at lines 301–302.
Confirm whether:
- The display label rebranding applies to all languages (Spanish should also change), or
- This is language-specific and Spanish retains the original labels
packages/visual-editor/src/components/contentBlocks/GetDirections.tsx (3)
2-2: LGTM: Supporting infrastructure for color selection.The imports, type definition, field configuration, and default props properly establish color support for GetDirections. The color field uses
SITE_COLORoptions and provides a sensible default fallback tobackgroundColors.color1.value.Also applies to: 4-12, 16-16, 24-27, 53-53
30-30: LGTM: Clean color prop handling.The component correctly destructures the color prop, provides a fallback using nullish coalescing, and passes the resolved color to the CTA. This approach ensures a default is always available while respecting user-selected colors.
Also applies to: 32-32, 43-43
55-61: Previous issue resolved: resolveFields now uses correct path.The resolveFields logic correctly uses
"color.visible"to control field visibility based on the selected variant. The fix addresses the major issue flagged in the previous review (which used the incorrect nested path"styles.objectFields.color.visible"). The implementation properly shows the color picker for button-style variants (primary/secondary) and hides it for link variants, aligning with expected UX behavior.packages/visual-editor/locales/hr/visual-editor.json (1)
189-189: Translation of new color field is accurate."Boja" is the correct Croatian translation for "Color."
Also applies to: 189-189
packages/visual-editor/src/components/contentBlocks/Address.tsx
Outdated
Show resolved
Hide resolved
...ing/screenshots/HeroSection/[desktop] [immersive] version 17 props using constant values.png
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/visual-editor/locales/nl/visual-editor.json (1)
301-302: Improve Dutch translations for CTA variant labels.While the empty-string issue has been addressed, the chosen translations have semantic issues:
- "outline" → "Overzicht": This means "overview," which doesn't convey the button-style concept. For a bordered/outline button variant, consider "Omtrek", "Contour", or "Rand" (border).
- "solid" → "Stevig": Translates awkwardly as "sturdy." The previous review suggested "Effen" (solid/filled), which is more idiomatic for UI button styles.
Update these to match standard Dutch UI terminology so Dutch users see meaningful labels.
Apply this diff for better translations:
"ctaVariant": { "link": "Link", - "outline": "Overzicht", - "solid": "Stevig" + "outline": "Omtrek", + "solid": "Effen" }
🧹 Nitpick comments (6)
packages/visual-editor/locales/ja/visual-editor.json (1)
301-302: Consider more semantically appropriate Japanese translations for CTA variant labels.The translations for the CTA variant options use "概要" (overview/summary) for "outline" and "固体" (solid in the physics sense) for "solid". While non-empty and technically comprehensible, these may not clearly convey the UI design intent to Japanese users.
For better UX clarity, consider using:
"outline": "アウトライン" (outline in katakana) or "枠線" (border/frame line) — more clearly represents a border-only button style"solid": "塗りつぶし" (filled/solid-filled) or "ソリッド" (solid in katakana) — more idiomatic for filled button styling in UI contextsNote: A top-level "outline" key also exists at line 556 with the same translation; ensure consistency across all usages if you update these.
packages/visual-editor/locales/sv/visual-editor.json (1)
190-190: CTA variant translations now in place; consider more idiomatic Swedish alternatives.The previous issue of missing Swedish translations for CTA variants has been resolved. Line 190 adds the new
colorfield with the correct translation "Färg", and lines 302–304 now provide translations for the variant options.However, the current translations use verb forms and colloquial terms that could be more aligned with Swedish UI conventions:
"outline": "Skissera"– Currently a verb (to sketch); consider"Kontur"(contour/outline) for a more natural UI label."solid": "Fast"– Currently means hard/solid in general sense; consider"Fylld"(filled) for consistency with typical UI terminology.These are optional improvements—the current translations are functional—but more idiomatic wording would improve consistency with Swedish interface conventions.
Also applies to: 302-304
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
981-1255: Consider refactoring to reduce test duplication.This test case is nearly identical to the "version 29 - various CTAs" test (lines 736–980), differing only in the addition of
colorproperties. The ~275 lines of duplicated structure create a maintenance burden: changes to the base CTA test structure will require updates in both places.Consider refactoring approaches:
- Helper function approach: Extract a factory function that generates the base test structure and optionally adds color properties:
function createCTATestCase(version: number, withColors: boolean) { const baseColors = withColors ? { color2: backgroundColors.color2.value, color1: backgroundColors.color1.value, color3: backgroundColors.color3.value, color4: backgroundColors.color4.value, } : {}; return { name: `version ${version} - ${withColors ? 'CTAs with different site colors' : 'various CTAs'}`, // ... build test structure using baseColors }; }
- Test parameterization: Use
it.eachwith a parameter to toggle color support:it.each([ { version: 29, useColors: false }, { version: 45, useColors: true }, ])("version $version CTAs", ({ version, useColors }) => { // ... conditionally add colors based on useColors });Either approach would reduce duplication and improve maintainability.
Note: The AI summary indicates that axe accessibility checks were removed from this test file. If true, this represents a regression in accessibility test coverage that should be reconsidered.
packages/visual-editor/src/components/contentBlocks/Address.tsx (3)
60-79: Consider typing the color field explicitly asBackgroundStyleThe
AddressStyleFieldswiring looks good and reusing it across blocks will help keep things in sync. For the color field:color: YextField<BackgroundStyle>(msg("fields.color", "Color"), { type: "select", options: "SITE_COLOR", }),would make the value type explicit and consistent with
AddressProps["styles"].color, similar to howctaVariantusesYextField<CTAVariant>. This is just for stronger TS checking and readability.
99-100: Confirm whethercolorshould affect non-primary/secondary variants
resolvedColoralways resolves to a site color:const resolvedColor = styles.color ?? backgroundColors.color1.value;and is passed to
CTAregardless ofctaVariant:<CTA ... variant={styles.ctaVariant} color={resolvedColor} />Given that the editor only shows the color field for
primary/secondary, can you confirm that passing a color for other variants (e.g."link") is intended and either used or safely ignored byCTA? If color should only matter forprimary/secondary, you might want to gateresolvedColoror thecolorprop on those variants.Also applies to: 149-160
194-201: Minor hardening ofresolveFieldsagainst missing stylesThe visibility toggling logic is clear and matches the requirement to only show color for
primary/secondary:const ctaVariant = data.props.styles.ctaVariant; const showColor = ctaVariant === "primary" || ctaVariant === "secondary"; setDeep(updatedFields, "styles.objectFields.color.visible", showColor);If there’s any chance
data.propsordata.props.stylescan be absent (e.g., partially-initialized state), consider a defensive fallback:const ctaVariant = data.props?.styles?.ctaVariant ?? "link";so
resolveFieldsdoesn’t throw in odd states and will simply hide the color field by default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (21)
packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (29)
packages/visual-editor/locales/cs/visual-editor.json(2 hunks)packages/visual-editor/locales/da/visual-editor.json(2 hunks)packages/visual-editor/locales/de/visual-editor.json(2 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(2 hunks)packages/visual-editor/locales/en/visual-editor.json(2 hunks)packages/visual-editor/locales/es/visual-editor.json(2 hunks)packages/visual-editor/locales/et/visual-editor.json(2 hunks)packages/visual-editor/locales/fi/visual-editor.json(2 hunks)packages/visual-editor/locales/fr/visual-editor.json(2 hunks)packages/visual-editor/locales/hr/visual-editor.json(2 hunks)packages/visual-editor/locales/hu/visual-editor.json(2 hunks)packages/visual-editor/locales/it/visual-editor.json(2 hunks)packages/visual-editor/locales/ja/visual-editor.json(2 hunks)packages/visual-editor/locales/lt/visual-editor.json(2 hunks)packages/visual-editor/locales/lv/visual-editor.json(2 hunks)packages/visual-editor/locales/nb/visual-editor.json(2 hunks)packages/visual-editor/locales/nl/visual-editor.json(2 hunks)packages/visual-editor/locales/pl/visual-editor.json(2 hunks)packages/visual-editor/locales/pt/visual-editor.json(2 hunks)packages/visual-editor/locales/ro/visual-editor.json(2 hunks)packages/visual-editor/locales/sk/visual-editor.json(2 hunks)packages/visual-editor/locales/sv/visual-editor.json(2 hunks)packages/visual-editor/locales/tr/visual-editor.json(2 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(2 hunks)packages/visual-editor/locales/zh/visual-editor.json(2 hunks)packages/visual-editor/src/components/atoms/cta.tsx(5 hunks)packages/visual-editor/src/components/contentBlocks/Address.tsx(8 hunks)packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx(2 hunks)packages/visual-editor/src/utils/themeConfigOptions.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/hr/visual-editor.json
- packages/visual-editor/locales/es/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/src/components/atoms/cta.tsx
- packages/visual-editor/locales/ro/visual-editor.json
- packages/visual-editor/src/utils/themeConfigOptions.ts
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/locales/fr/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
🧰 Additional context used
🧬 Code graph analysis (2)
packages/visual-editor/src/components/contentBlocks/Address.tsx (4)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/utils/index.ts (2)
BackgroundStyle(33-33)backgroundColors(30-30)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-327)packages/visual-editor/src/editor/index.ts (1)
resolveDataFromParent(28-28)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
backgroundColors(50-125)
⏰ 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 (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (15)
packages/visual-editor/locales/sk/visual-editor.json (1)
190-190: Translations properly addressed.The previous issue flagging missing translations for "solid" and "outline" has been resolved. The Slovak translations are now in place: "Pevné" (solid) and "Obrys" (outline). The new "color" field is also properly localized as "Farba." All locale entries align with the cross-locale changes for the CTA color selection feature.
Also applies to: 301-303
packages/visual-editor/locales/it/visual-editor.json (1)
189-189: Italian translations are complete and properly formatted.The new
colorfield and updated CTA variant options (outlineandsolid) all have appropriate Italian translations:
- "Colore" (Color)
- "Contorno" (Outline)
- "Solido" (Solid)
This resolves the previous issue flagged about empty translations.
Also applies to: 300-302
packages/visual-editor/locales/lt/visual-editor.json (1)
189-189: Locale translations properly completed.The Lithuanian translations for the new
colorfield and updated CTA variant labels are now in place:
- Line 189:
"color": "Spalva"✓- Lines 301–302:
"outline": "Kontūras"and"solid": "Tvirtas"✓This addresses the previous concern about empty translations. The prior flag has been resolved.
Also applies to: 301-302
packages/visual-editor/locales/zh-TW/visual-editor.json (2)
189-189: Color field translation looks good.The translation "顏色" correctly represents the color field.
301-302: Verify the "solid" variant translation.The translation "堅硬的" for the "solid" variant may not be the most contextually appropriate in a UI button/variant context. In Traditional Chinese, "solid" styling typically refers to a filled button or solid color, which might be better expressed as "實色" (solid color) or "填充" (filled) rather than "堅硬的" (hard/firm).
Please verify this translation aligns with:
- The intended visual design meaning (a filled/solid-background button variant)
- Terminology used in other locale files and existing design system documentation
- UX conventions in your target audience's language
packages/visual-editor/locales/da/visual-editor.json (2)
195-195: New color field translation looks good.The Danish translation "Farve" correctly represents the new color field addition across the CTA components.
306-310: Verify CTA variant translation and underlying value alignment.Two concerns with the variant options update:
Translation quality of "Disposition": The Danish word "Disposition" (meaning layout/arrangement/temperament) may not clearly convey the visual concept of an outlined/bordered button. More appropriate options might be "Kontur" (outline/contour), "Ramme" (frame/border), or "Linje" (line). Please confirm this translation is correct.
Discrepancy with PR objectives: The PR description states "underlying values remain 'primary' and 'secondary'", but the code shows keys changed to
"outline"and"solid". Please clarify whether this is an intentional structural change or if the values should actually be"primary"and"secondary"with display labels showing "Solid" and "Disposition".packages/visual-editor/locales/et/visual-editor.json (1)
189-189: Localization changes align with PR objectives.The Estonian locale has been correctly updated to support the new color selection feature:
- Line 189: New
colorfield added with translation "Värv"- Lines 301-302: CTA variant options renamed from primary/secondary to outline/solid, with appropriate Estonian translations ("Kontuur" and "Tahke")
The scope is appropriate—other references to primary/secondary in different contexts (e.g.,
primaryCTAfield names, color theme labels) remain unchanged, which is correct given the PR's statement that underlying structural values persist elsewhere.Also applies to: 299-303
packages/visual-editor/locales/en-GB/visual-editor.json (2)
188-188: New color field translation looks good.The translation addition aligns with the new color-selection feature. Placement is alphabetically consistent within the fields object.
298-302: CTA variant labels correctly updated.The rename from "Primary"/"Secondary" to "Solid"/"Outline" is consistent with the PR objectives and maintains the "link" option unchanged across all 25 locale files (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). No regional label inconsistencies exist.
packages/visual-editor/locales/zh/visual-editor.json (1)
299-303: No action required. The ctaVariant keys in the locale file are correct as-is.The locale file structure properly uses display label keys ("link", "outline", "solid") which map to translated UI strings. The underlying component values ("primary", "secondary", "link") are correctly defined in
themeConfigOptions.ts, wheremsg("fields.options.ctaVariant.solid", "Solid")returns the translated label while mapping to the underlying value"primary". This is the standard pattern for UI localization where display labels are separated from underlying values. All locale files across the codebase consistently use this same structure.packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
5-5: LGTM!The import of
backgroundColorsis necessary to support the new test case validating site color selection for CTAs.packages/visual-editor/locales/en/visual-editor.json (1)
188-188: New color field label added correctly.The addition of
"color": "Color"on line 188 aligns with the CTA color-selection feature and follows the existing field label convention.packages/visual-editor/src/components/contentBlocks/Address.tsx (2)
35-41: Style API change for color looks consistent with theme typesAdding
color?: BackgroundStyletoAddressProps.stylesaligns with theBackgroundStyledefinition and keeps the prop optional for older content while still allowing a default viadefaultProps. No issues here.
191-192: Default color is aligned with runtime fallbackSetting:
color: backgroundColors.color1.value,in
defaultProps.stylesmatches the runtime fallback inresolvedColor, so new blocks and older content without an explicit color will behave consistently. This looks good.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/visual-editor/locales/tr/visual-editor.json (1)
301-302: CTA variant translations could use more idiomatic terminology.The prior issue with empty strings has been resolved. However, "Taslak" (draft) is not the most idiomatic Turkish term for the "Outline" button variant. In design contexts, "Anahat" (outline/contour) would be more appropriate and clearer to Turkish users. Consider updating line 302 to use "Anahat" instead of "Taslak" for better terminology consistency with standard design terminology.
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
163-181: SITE_COLOR options wiring and prefix stripping look goodDefining
siteColorOptionsoffbackgroundColorsand strippingbg-/text-prefixes here, then exposing it asThemeOptions.SITE_COLOR, is consistent with how other theme groups are modeled and keeps the normalization logic centralized. I don’t see any correctness or API issues with this approach.If you ever need the same normalization elsewhere, consider extracting a small helper (e.g.,
stripTailwindPrefixes) nearBackgroundStyleso both SITE_COLOR and any future consumers can share it.Also applies to: 408-414
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (26)
packages/visual-editor/locales/cs/visual-editor.json(2 hunks)packages/visual-editor/locales/da/visual-editor.json(2 hunks)packages/visual-editor/locales/de/visual-editor.json(2 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(2 hunks)packages/visual-editor/locales/en/visual-editor.json(2 hunks)packages/visual-editor/locales/es/visual-editor.json(2 hunks)packages/visual-editor/locales/et/visual-editor.json(2 hunks)packages/visual-editor/locales/fi/visual-editor.json(2 hunks)packages/visual-editor/locales/fr/visual-editor.json(2 hunks)packages/visual-editor/locales/hr/visual-editor.json(2 hunks)packages/visual-editor/locales/hu/visual-editor.json(2 hunks)packages/visual-editor/locales/it/visual-editor.json(2 hunks)packages/visual-editor/locales/ja/visual-editor.json(2 hunks)packages/visual-editor/locales/lt/visual-editor.json(2 hunks)packages/visual-editor/locales/lv/visual-editor.json(2 hunks)packages/visual-editor/locales/nb/visual-editor.json(2 hunks)packages/visual-editor/locales/nl/visual-editor.json(2 hunks)packages/visual-editor/locales/pl/visual-editor.json(2 hunks)packages/visual-editor/locales/pt/visual-editor.json(2 hunks)packages/visual-editor/locales/ro/visual-editor.json(2 hunks)packages/visual-editor/locales/sk/visual-editor.json(2 hunks)packages/visual-editor/locales/sv/visual-editor.json(2 hunks)packages/visual-editor/locales/tr/visual-editor.json(2 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(2 hunks)packages/visual-editor/locales/zh/visual-editor.json(2 hunks)packages/visual-editor/src/utils/themeConfigOptions.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/visual-editor/locales/fr/visual-editor.json
- packages/visual-editor/locales/en/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/lt/visual-editor.json
- packages/visual-editor/locales/es/visual-editor.json
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/da/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/it/visual-editor.json
⏰ 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 (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (10)
packages/visual-editor/locales/lv/visual-editor.json (1)
189-189: Localization completeness verified.The Latvian translations for the new and updated fields are complete and non-empty:
- Line 189:
"color": "Krāsa"(Color) is properly translated.- Lines 301–302:
"primary": "Ciets"(Solid) and"secondary": "Kontūra"(Outline) are complete and address the prior review's concern about empty translations.This resolves the issue flagged in the previous review about blank UI labels.
Also applies to: 301-302
packages/visual-editor/locales/pt/visual-editor.json (1)
189-189: Translations look correct and complete.The three changes—adding the
colorfield label and updating CTA variant display labels—are properly translated to Portuguese and align with the PR objectives. The underlying keys correctly remain "primary"/"secondary" while the displayed labels now show "Sólido" and "Contorno". The past review concern about empty translation strings has been addressed.Also applies to: 301-302
packages/visual-editor/locales/cs/visual-editor.json (3)
194-194: Czech translation for color field is well-placed.The new "color": "Barva" entry is correctly positioned alphabetically and follows the field naming convention.
308-309: Czech translations for CTA variant labels are appropriate.The updates from "Primární"/"Sekundární" to "Solidní"/"Obrys" align with the PR's intent to rename display labels from Primary/Secondary to Solid/Outline. The Czech translations are semantically correct.
1-749: Verify consistency across all locale files.This Czech locale file has been updated correctly, but ensure that all other locale files (en, de, fr, pt, ro, sk, sv, tr, zh-CN, zh-TW, etc.) mentioned in the PR summary have equivalent changes to maintain consistency across the UI.
packages/visual-editor/locales/hr/visual-editor.json (2)
189-189: Translation for newcolorfield is correct.The Croatian translation "Boja" accurately reflects the new color-selection feature added in this PR. No issues.
299-303: CTA variant label translations correctly updated.The translation values have been properly updated to reflect the display label changes:
"primary": "Čvrsto"(Solid) ✓"secondary": "Obris"(Outline) ✓The underlying keys remain "primary" and "secondary" as intended per the PR objectives. The translations are accurate and consistent with the label rename pattern.
packages/visual-editor/locales/nb/visual-editor.json (1)
189-189: Verify Norwegian translations for "Solid" and "Outline" CTA variants.Line 189 correctly adds the "color" field translation ("Farge"). However, the CTA variant translations at lines 301–302 may need clarification:
"primary": "Fast"(changed from "Primær") — "Fast" typically means "fixed" or "firm" in Norwegian, which may not clearly convey "Solid" to end users."secondary": "Disposisjon"(changed from "Sekundær") — "Disposisjon" typically means "arrangement," which may confuse users expecting a term closer to "Outline" or "Border."A previous review suggested Norwegian terms like "Fyll" (filled) for "Solid" and "Omriss" (outline) or "Kontur" (contour) for "Outline," which may be more intuitive for Norwegian speakers.
Please confirm these translation choices are intentional, or consider revising them to more intuitive Norwegian equivalents that better reflect the visual behavior of the button variants.
Also applies to: 301-302
packages/visual-editor/locales/tr/visual-editor.json (1)
189-189: Color field translation added correctly.The new "color" field translation "Renk" is accurate.
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
291-300: CTA variant label rename is safe and aligned with specUpdating the labels to "Solid" and "Outline" while keeping the keys and
valuestrings as"primary"/"secondary"preserves existing behavior and storage while matching the new UX copy. All good here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/visual-editor/src/components/contentBlocks/Address.tsx (2)
99-99: Avoid duplicating the default CTA color literal
backgroundColors.color1.valueis currently hardcoded both inresolvedColor(Line 99) and indefaultProps.styles.color(Line 190). This works, but it’s easy for these to drift if someone later changes only one of them.Consider extracting a single module-level constant and reusing it:
+const DEFAULT_ADDRESS_CTA_COLOR = backgroundColors.color1.value; + const AddressComponent: PuckComponent<AddressProps> = (props) => { const { data, styles, puck, parentData } = props; … - const resolvedColor = styles.color ?? backgroundColors.color1.value; + const resolvedColor = styles.color ?? DEFAULT_ADDRESS_CTA_COLOR; … } export const Address: ComponentConfig<{ props: AddressProps }> = { … defaultProps: { … styles: { showGetDirectionsLink: true, ctaVariant: "link", - color: backgroundColors.color1.value, + color: DEFAULT_ADDRESS_CTA_COLOR, }, }, … };This keeps the default behavior identical while reducing the chance of inconsistencies.
Also applies to: 150-159, 187-191
193-200: MakeresolveFieldsresilient to partial or legacy dataThe visibility logic for the color field based on
ctaVariantis correct, but accessingdata.props.styles.ctaVariantassumespropsandstylesare always present and fully populated. IfresolveFieldsis ever invoked with partial/legacy data (or before defaults are fully merged), this could throw.You can make this a bit more defensive with a cheap fallback, without changing behavior for valid data:
- resolveFields(data) { - const updatedFields = resolveDataFromParent(addressFields, data); - const ctaVariant = data.props.styles.ctaVariant; - const showColor = ctaVariant === "primary" || ctaVariant === "secondary"; + resolveFields(data) { + const updatedFields = resolveDataFromParent(addressFields, data); + const ctaVariant = data?.props?.styles?.ctaVariant ?? "link"; + const showColor = ctaVariant === "primary" || ctaVariant === "secondary"; setDeep(updatedFields, "styles.objectFields.color.visible", showColor); return updatedFields; },If the editor guarantees full
data.props.stylesat all times, this is non-essential, but it hardens the config against malformed content and future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (21)
packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/contentBlocks/Address.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/contentBlocks/Address.tsx (2)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/utils/index.ts (2)
BackgroundStyle(33-33)backgroundColors(30-30)
⏰ 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 (1)
packages/visual-editor/src/components/contentBlocks/Address.tsx (1)
2-7: Color theming wiring for Address styles looks consistent and type-safeThe introduction of
color?: BackgroundStyleonAddressProps.styles, the typedAddressStyleFieldsexport, and the newcolorselect field (using the site-color options) all line up cleanly with the existing styles schema and sharedbackgroundColorspalette. ReusingAddressStyleFieldsinaddressFields.styles.objectFieldskeeps the config DRY and strongly typed. I don’t see any correctness issues here.Also applies to: 23-25, 35-41, 60-79, 81-92
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/visual-editor/src/components/pageSections/PromoSection.test.tsx (1)
726-729: Tighten typing and consider sharing the brand-color helper
isBrandColorTestis functionally fine (defensive optional chaining and string check), but a couple of small improvements would help:
- Avoid
anyforprops; you can type this asComponentTest["props"]or a minimal{ styles?: { backgroundColor?: { bgColor?: string } } }shape to keep tests aligned with the component contract.- This brand-color detection logic is likely duplicated in other tests (e.g., HeroSection, Grid). Extracting a shared helper in your testing utilities would avoid drift if token naming or brand-color rules change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (18)
packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[desktop] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[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/CoreInfoSection/[desktop] version 1 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[desktop] version 1 props with partial entity values 1.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[desktop] version 1 props with partial entity values 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[desktop] version 27 props with mixed values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[mobile] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[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/CoreInfoSection/[mobile] version 1 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[mobile] version 1 props with partial entity values 1.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[mobile] version 1 props with partial entity values 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[mobile] version 27 props with mixed values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[tablet] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[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/CoreInfoSection/[tablet] version 1 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[tablet] version 1 props with partial entity values 1.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[tablet] version 1 props with partial entity values 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/CoreInfoSection/[tablet] version 27 props with mixed values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (2)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx(2 hunks)packages/visual-editor/src/components/pageSections/PromoSection.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/visual-editor/src/components/pageSections/PromoSection.test.tsx (1)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
axe(77-81)
packages/visual-editor/src/components/pageSections/HeroSection.test.tsx (1)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
axe(77-81)
⏰ 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 (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
packages/visual-editor/src/components/pageSections/PromoSection.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 45 - CTAs with Dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 45 - CTAs with Dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 29 - various CTAs.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 45 - CTAs with Dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 45 - CTAs with different site colors.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (7)
packages/visual-editor/src/components/atoms/cta.tsx(5 hunks)packages/visual-editor/src/components/contentBlocks/Address.tsx(8 hunks)packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx(6 hunks)packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx(7 hunks)packages/visual-editor/src/components/contentBlocks/GetDirections.tsx(2 hunks)packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx(3 hunks)packages/visual-editor/src/utils/themeConfigOptions.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/visual-editor/src/components/atoms/cta.tsx
- packages/visual-editor/src/utils/themeConfigOptions.ts
🧰 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/components/contentBlocks/CtaWrapper.tsx
🧬 Code graph analysis (3)
packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx (3)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/utils/index.ts (2)
BackgroundStyle(33-33)backgroundColors(30-30)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-327)
packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx (2)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-327)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
backgroundColors(50-125)
🔇 Additional comments (2)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (2)
5-5: v45 CTA color fixtures are wired correctly but depend on fixing CTA color handlingThe new v45 test cases nicely cover CTAs with different
backgroundColors.colorX.valueand a dark section background, and the addedbackgroundColorsimport is used consistently. Note that, as written, the CTA components never actually passcolorthrough to<CTA>due to the sharedisDefaultlogic in CTAWrapper/CTAGroup/GetDirections/Address; once that is corrected, these v45 screenshots will likely change and should be regenerated to validate the color behavior end‑to‑end.Also applies to: 982-1531
1591-1613: Conditional axe assertions for v45 scenarios look goodLimiting
axefailures to existing fixtures while logging results (instead of asserting) for the two known problematic v45 cases is a reasonable compromise and keeps the rest of the accessibility suite intact.
packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/visual-editor/locales/nl/visual-editor.json (1)
303-303: Semantic mismatch in Dutch translation for secondary CTA variant.The translation "Overzicht" (Overview) is semantically incorrect for the secondary variant. Per the PR objectives, this variant represents an "Outline" button style, not an overview. Consider updating to "Omtrek" (outline) or "Contour" to accurately convey the button styling to Dutch users.
Apply this diff:
- "secondary": "Overzicht" + "secondary": "Omtrek"
🧹 Nitpick comments (2)
packages/visual-editor/locales/lv/visual-editor.json (1)
302-303: CTA variant labels updated to match new terminology.The translations have been updated from "Primārs"/"Sekundārs" to "Ciets"/"Kontūra", reflecting the UI label change from Primary/Secondary to Solid/Outline. The previous issue with empty translations has been addressed.
While "Ciets" (solid) and "Kontūra" (outline) appear to be reasonable Latvian translations, consider having these verified by a native Latvian speaker or professional translator to ensure they convey the intended UI semantics clearly to end users.
packages/visual-editor/src/components/contentBlocks/GetDirections.tsx (1)
47-62: Consider whether default color should be provided whencolorprop is undefined.The
defaultProps.coloris set tobackgroundColors.color1.value, but inGetDirectionsComponent,resolvedColor = colormeans if a user explicitly clears the color selection, it could becomeundefined. This differs slightly from the previous pattern where a fallback was used (color ?? backgroundColors.color1.value).If the intent is to always have a color applied for solid/outline variants, consider:
- const resolvedColor = color; + const resolvedColor = color ?? backgroundColors.color1.value;However, if allowing
undefinedto fall back to CTA's internal theming is intentional, the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
packages/visual-editor/locales/cs/visual-editor.json(3 hunks)packages/visual-editor/locales/da/visual-editor.json(3 hunks)packages/visual-editor/locales/de/visual-editor.json(3 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(3 hunks)packages/visual-editor/locales/en/visual-editor.json(3 hunks)packages/visual-editor/locales/es/visual-editor.json(3 hunks)packages/visual-editor/locales/et/visual-editor.json(3 hunks)packages/visual-editor/locales/fi/visual-editor.json(3 hunks)packages/visual-editor/locales/fr/visual-editor.json(3 hunks)packages/visual-editor/locales/hr/visual-editor.json(3 hunks)packages/visual-editor/locales/hu/visual-editor.json(3 hunks)packages/visual-editor/locales/it/visual-editor.json(3 hunks)packages/visual-editor/locales/ja/visual-editor.json(3 hunks)packages/visual-editor/locales/lt/visual-editor.json(3 hunks)packages/visual-editor/locales/lv/visual-editor.json(3 hunks)packages/visual-editor/locales/nb/visual-editor.json(3 hunks)packages/visual-editor/locales/nl/visual-editor.json(3 hunks)packages/visual-editor/locales/pl/visual-editor.json(3 hunks)packages/visual-editor/locales/pt/visual-editor.json(3 hunks)packages/visual-editor/locales/ro/visual-editor.json(3 hunks)packages/visual-editor/locales/sk/visual-editor.json(3 hunks)packages/visual-editor/locales/sv/visual-editor.json(3 hunks)packages/visual-editor/locales/tr/visual-editor.json(3 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(3 hunks)packages/visual-editor/locales/zh/visual-editor.json(3 hunks)packages/visual-editor/src/components/contentBlocks/Address.tsx(8 hunks)packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx(6 hunks)packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx(7 hunks)packages/visual-editor/src/components/contentBlocks/GetDirections.tsx(2 hunks)packages/visual-editor/src/utils/themeConfigOptions.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/src/components/contentBlocks/Address.tsx
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/es/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/locales/hr/visual-editor.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-15T20:59:20.838Z
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:20.838Z
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/contentBlocks/CTAGroup.tsxpackages/visual-editor/locales/lv/visual-editor.json
📚 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/contentBlocks/CtaWrapper.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/locales/hu/visual-editor.json
🧬 Code graph analysis (3)
packages/visual-editor/src/components/contentBlocks/GetDirections.tsx (6)
packages/visual-editor/src/components/contentBlocks/index.ts (1)
GetDirectionsProps(6-6)packages/visual-editor/src/components/atoms/cta.tsx (2)
CTAVariant(48-48)CTA(148-269)packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/utils/index.ts (2)
BackgroundStyle(33-33)backgroundColors(30-30)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-327)packages/visual-editor/src/editor/index.ts (1)
resolveDataFromParent(28-28)
packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx (2)
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
BackgroundStyle(36-43)backgroundColors(50-125)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-327)
packages/visual-editor/src/utils/themeConfigOptions.ts (4)
packages/visual-editor/src/internal/puck/ui/Combobox.tsx (1)
ComboboxOptionGroup(23-27)packages/visual-editor/src/utils/i18n/platform.ts (1)
msg(104-104)packages/visual-editor/src/utils/i18n/index.ts (1)
msg(5-5)packages/visual-editor/src/utils/index.ts (1)
backgroundColors(30-30)
🔇 Additional comments (28)
packages/visual-editor/locales/nl/visual-editor.json (3)
146-146: LGTM!The Dutch translation "Standaard" for "default" is correct.
190-190: LGTM!The Dutch translation "Kleur" for the color field is correct.
302-302: LGTM!The Dutch translation "Stevig" (solid/sturdy) is appropriate for the primary CTA variant representing a "Solid" button style.
packages/visual-editor/locales/pt/visual-editor.json (3)
146-146: LGTM! Accurate translation for "default"."Padrão" is the correct and commonly used Portuguese translation for "Default" in UI contexts.
190-190: LGTM! Correct translation for color field."Cor" is the accurate Portuguese translation for "Color", properly supporting the new color selection feature.
302-303: LGTM! Appropriate UI terminology for CTA variants.The updated translations "Sólido" (Solid) and "Contorno" (Outline) are contextually accurate and commonly used terms in Portuguese UI/UX for button styles. This properly reflects the display label changes from Primary/Secondary to Solid/Outline while maintaining the underlying values.
packages/visual-editor/locales/lv/visual-editor.json (2)
146-146: LGTM: Default key translation added.The addition of the
"default": "Noklusējums"key is appropriate and aligns with the broader localization changes across other locale files in this PR.
190-190: LGTM: Color field translation added.The
"color": "Krāsa"entry correctly adds translation support for the new color-selection field introduced in this PR.packages/visual-editor/locales/hu/visual-editor.json (3)
146-146: LGTM! Correct translation for "default".The translation "Alapértelmezett" is accurate and appropriate for the default option label.
190-190: LGTM! Correct translation for the new color field.The translation "Szín" (color) is accurate and aligns with the PR's objective to introduce color-selection support for CTA components.
302-303: LGTM! CTA variant labels updated correctly.The translations correctly implement the PR's objective to update display labels from "Primary"/"Secondary" to "Solid"/"Outline" while maintaining the underlying "primary" and "secondary" values:
- "Szilárd" (Solid) - accurate translation, consistent with other locales (French "Solide", Spanish "Sólido")
- "Vázlat" (Outline) - appropriate for button style context, similar to Czech "Obrys" and Estonian "Kontuur"
The empty string issue flagged in the previous review has been successfully resolved.
Based on past review comments, this addresses the previously identified translation gap.
packages/visual-editor/locales/fi/visual-editor.json (1)
146-146: LGTM! Translation keys added correctly.The new
defaultandcolorkeys are properly translated to Finnish ("Oletus" and "Väri" respectively) and align with the PR's objective of adding color-selection support for CTA components.Also applies to: 190-190
packages/visual-editor/locales/ja/visual-editor.json (2)
146-146: LGTM!The translation "デフォルト" is the standard Japanese katakana transliteration for "default" and is commonly used in UI contexts.
190-190: LGTM!The translation "色" (iro) is the correct and natural Japanese word for "color" in UI contexts.
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
299-309: LGTM: CTA variant label updates.The updated labels ("Solid" and "Outline") provide clearer, more descriptive names than the previous "Primary" and "Secondary", improving UX consistency across the visual editor.
163-189: Format inconsistency is already handled by the existingnormalize()function.The CTA component's
normalize()function in cta.tsx already handles both prefixed and unprefixed color formats correctly. It strips"bg-"and"text-"prefixes when present, returning the value unchanged if no prefix exists. This means:
- Default colors from
backgroundColors.color1.value(e.g.,{ bgColor: "bg-palette-primary", ... })- User-selected colors from
siteColorOptions(e.g.,{ bgColor: "palette-primary", ... })Both formats converge to the same unprefixed values after
normalize()is applied, ensuring consistent color handling throughout the component. No further changes needed.packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx (2)
81-84: Note: Color format consistency with siteColorOptions.This field uses
SITE_COLORoptions, which strip the"bg-"and"text-"prefixes from color values (per themeConfigOptions.ts lines 179-180). However, the default color (line 45) usesbackgroundColors.color1.value, which retains these prefixes. Ensure the CTA component consistently handles both formats.See verification script in the themeConfigOptions.ts review comment.
45-45: The CTA component gracefully handlescolor={undefined}through optional chaining (color?.bgColorandcolor?.textColoron lines 162-164) and conditional operators that prevent undefined values from being applied to styles (lines 169-171, 177-179). For existing CTAs without a color property, no dynamic styling is applied, but the component renders without error—backward compatible rendering is assured.packages/visual-editor/src/components/contentBlocks/GetDirections.tsx (4)
2-12: LGTM!The imports are correctly organized, adding the necessary dependencies for color support (
BackgroundStyle,backgroundColors) and field visibility logic (setDeep,resolveDataFromParent).
14-17: LGTM!The
colorprop is correctly typed as optionalBackgroundStyle, aligning with the pattern used in other CTA components.
19-28: LGTM!The color field is correctly configured with
SITE_COLORoptions, matching the pattern established inCtaWrapper.tsx.
30-45: Color is now correctly passed to CTA.The previous issue where
isDefaultcheck caused color to be a no-op has been addressed.resolvedColoris now passed directly to theCTAcomponent.packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx (6)
23-25: LGTM!The imports for
BackgroundStyleandbackgroundColorsare correctly added to support the new color functionality.
46-46: LGTM!The
colorproperty is correctly typed as optionalBackgroundStylewithin the styles interface.
109-113: LGTM!The color field configuration is consistent with the pattern used in
GetDirections.tsx.
145-146: Color is now correctly passed to CTA.The previous issue where the
isDefaultcheck caused color to never be applied has been addressed.resolvedColoris now passed directly to theCTAcomponent.Also applies to: 172-172
201-201: LGTM!Default color is correctly set to
backgroundColors.color1.value.
222-226: LGTM!The
showColorlogic correctly hides the color field when:
- The variant is not
primaryorsecondary(i.e., when it'slink)- The
ctaTypeispresetImage(where color styling doesn't apply)This aligns with the expected behavior for color selection visibility.
|
@benlife5 - It looks something is off with |
packages/visual-editor/src/components/contentBlocks/CTAGroup.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/contentBlocks/CtaWrapper.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/contentBlocks/GetDirections.tsx
Outdated
Show resolved
Hide resolved
|
The NearbyLocations test requires an API key to run locally. The failure on GitHub was just a flakiness issue, it passed when re-run. Left a couple nitpicks but otherwise this LGTM |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/visual-editor/locales/zh-TW/visual-editor.json (1)
303-303: Consider using "實心" or "實色" instead of "堅硬的" for better clarity.The translation "堅硬的" literally means "hard" or "rigid," which doesn't clearly convey the UI concept of a solid/filled button style. In Traditional Chinese UI contexts, better alternatives would be:
- "實心" (solid-filled)
- "實色" (solid-color)
- "填滿" (filled)
These alternatives more accurately represent the visual style of a solid button versus an outline button.
Apply this diff to improve the translation:
"ctaVariant": { "link": "關聯", - "primary": "堅硬的", + "primary": "實心", "secondary": "大綱" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
packages/visual-editor/locales/cs/visual-editor.json(3 hunks)packages/visual-editor/locales/da/visual-editor.json(3 hunks)packages/visual-editor/locales/de/visual-editor.json(3 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(3 hunks)packages/visual-editor/locales/en/visual-editor.json(3 hunks)packages/visual-editor/locales/es/visual-editor.json(3 hunks)packages/visual-editor/locales/et/visual-editor.json(3 hunks)packages/visual-editor/locales/fi/visual-editor.json(3 hunks)packages/visual-editor/locales/fr/visual-editor.json(3 hunks)packages/visual-editor/locales/hr/visual-editor.json(3 hunks)packages/visual-editor/locales/hu/visual-editor.json(3 hunks)packages/visual-editor/locales/it/visual-editor.json(3 hunks)packages/visual-editor/locales/ja/visual-editor.json(3 hunks)packages/visual-editor/locales/lt/visual-editor.json(3 hunks)packages/visual-editor/locales/lv/visual-editor.json(3 hunks)packages/visual-editor/locales/nb/visual-editor.json(3 hunks)packages/visual-editor/locales/nl/visual-editor.json(3 hunks)packages/visual-editor/locales/pl/visual-editor.json(3 hunks)packages/visual-editor/locales/pt/visual-editor.json(3 hunks)packages/visual-editor/locales/ro/visual-editor.json(3 hunks)packages/visual-editor/locales/sk/visual-editor.json(3 hunks)packages/visual-editor/locales/sv/visual-editor.json(3 hunks)packages/visual-editor/locales/tr/visual-editor.json(3 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(3 hunks)packages/visual-editor/locales/zh/visual-editor.json(3 hunks)packages/visual-editor/src/docs/components.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/da/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/fr/visual-editor.json
- packages/visual-editor/src/docs/components.md
- packages/visual-editor/locales/lt/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/nl/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
⏰ 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 (17)
packages/visual-editor/locales/en-GB/visual-editor.json (2)
145-145: LGTM - New localization keys for color selection feature.The top-level
defaultkey and newcolorfield label are correctly added to support the site-color selection feature.Also applies to: 189-189
300-304: LGTM - CTA variant label updates.The display labels are appropriately renamed to "Solid"/"Outline" for better clarity while the underlying values (
primary/secondary) are retained, ensuring backward compatibility.packages/visual-editor/locales/cs/visual-editor.json (1)
151-151: Verify Czech translations with a native speaker.The translations appear technically correct:
- Line 151: "Výchozí" (Default)
- Line 195: "Barva" (Color)
- Lines 310-311: "Solidní" (Solid), "Obrys" (Outline)
However, please ensure these terms are natural and user-friendly in a Czech UI context, particularly the CTA variant labels "Solidní" and "Obrys".
Consider having a native Czech speaker or localization specialist review these translations to confirm they are idiomatic and appropriate for the UI context.
Also applies to: 195-195, 310-311
packages/visual-editor/locales/et/visual-editor.json (1)
146-146: LGTM! Estonian translations correctly implement the new features.The additions and updates are accurate:
- "Vaikimisi" (Default) for the new default label
- "Värv" (Color) for the color selection field
- "Tahke" (Solid) and "Kontuur" (Outline) for the updated CTA variant labels
These changes align with the PR objectives and maintain consistency with other locale files.
Also applies to: 190-190, 303-304
packages/visual-editor/locales/en/visual-editor.json (3)
145-145: LGTM! Good addition for localization.The new "default" translation key will be useful for UI elements that display default options or values.
189-189: LGTM! Supports the new color selection feature.The new "color" field translation aligns with the PR objective to add color-selection support for CTAs.
302-303: LGTM! CTA variant labels correctly updated.The display labels have been properly updated to "Solid" and "Outline" while correctly retaining the underlying keys "primary" and "secondary" that match the component implementation. This resolves the past critical issue and aligns perfectly with the PR objectives.
packages/visual-editor/locales/zh-TW/visual-editor.json (2)
146-146: LGTM! Correct translation for "default".The translation "預設" is accurate and commonly used in Traditional Chinese UIs for "default" options.
190-190: LGTM! Correct translation for the color field.The translation "顏色" accurately represents "color" and is properly placed within the fields section.
packages/visual-editor/locales/es/visual-editor.json (2)
145-145: LGTM!The translations for "default" ("Por defecto") and "color" ("Color") are correct and appropriate for Spanish-speaking users.
Also applies to: 189-189
302-302: LGTM!The translation "Sólido" correctly represents the "Solid" CTA variant style.
packages/visual-editor/locales/nb/visual-editor.json (1)
190-190: LGTM!The translation "Farge" for "color" is correct.
packages/visual-editor/locales/tr/visual-editor.json (2)
146-146: LGTM!The translation "Varsayılan" correctly represents "Default" in Turkish and is appropriate for the color selector context.
190-190: LGTM!The translation "Renk" correctly represents "Color" in Turkish and properly supports the new color selection feature.
packages/visual-editor/locales/hr/visual-editor.json (3)
146-146: LGTM! New default translation added.The Croatian translation "Zadano" for "default" is accurate and appropriately placed at the root level for use across UI components.
190-190: LGTM! Color field translation added.The Croatian translation "Boja" for the new color field is accurate and correctly placed under the
fieldsobject, supporting the color-selection feature.
303-304: No action needed. The translation keysprimaryandsecondaryalign with code expectations inpackages/visual-editor/src/utils/themeConfigOptions.ts, which uses the i18n pathsfields.options.ctaVariant.primaryandfields.options.ctaVariant.secondary. The Croatian translations "Čvrsto" (Solid) and "Obris" (Outline) are correct.
...isual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various atoms.png
Show resolved
Hide resolved
[Jira Ticket](https://yext.atlassian.net/jira/software/c/projects/OPAQF/boards/6188?label=ootb-component&selectedIssue=OPAQF-54). - Added color-selection support for. - CTAWrapper - CTAGroup - GetDirections - Address - cta atom. - Added a new Site-only colors field/dropdown. - Updated the **Secondary** and **Primary** _labels only_ to **Outline** and **Solid** and left the value as **primary** and **secondary**. - Dynamically hide/show the props based on the selected variant or cta type. Known issues: - Running i18n generates new props instead of overriding existing ones. Removing locale folders under /locales causes additional props to be created, so this was left unchanged. Please lemme know If I'll need to delete the locales order and re-run the i18n command. - CTAGroup still cannot use resolveFields for dynamic show/hide behaviour based on selected props . https://github.com/user-attachments/assets/66e7f08d-5aac-47fb-b202-4449b850d93f --------- 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>
4 participants
Jira Ticket.
Known issues:
Removing locale folders under /locales causes additional props to be created, so this was left unchanged. Please lemme know If I'll need to delete the locales order and re-run the i18n command.
Screen.Recording.2025-12-09.at.2.24.52.PM.mov