feat: added color options to heading text #963
Conversation
WalkthroughAdds localized keys Sequence Diagram(s)sequenceDiagram
participant User as Editor User
participant UI as Visual Editor UI
participant Config as themeConfigOptions
participant HeadingText as HeadingText block
participant Heading as Heading atom
participant Normalize as normalizeThemeColor
participant CTA as CTA atom
participant Styles as Runtime CSS vars
rect rgb(245,250,255)
Note right of UI: Locales updated with\n"recommendedColor" &\n"recommendedColorDescription"
UI->>Config: request siteColorOptions
Config-->>UI: return groups (Default/"Recommended" + programmatic site colors)
end
User->>UI: open heading style panel
UI->>User: show "Recommended Color" label & description
User->>UI: select color option
UI->>HeadingText: pass styles.color (BackgroundStyle)
HeadingText->>Heading: render with color prop
Heading->>Normalize: normalizeThemeColor(color.bgColor)
Normalize-->>Heading: theme color token (e.g., "blue-500") or undefined
Heading->>Styles: set CSS var from token (inline style)
Styles-->>Heading: computed color applied
Heading-->>UI: render updated heading
Note over CTA,Normalize: CTA also uses normalizeThemeColor for\nbackground/text tokens before render
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)packages/visual-editor/src/utils/normalizeThemeColor.test.ts (1)
⏰ 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). (4)
🔇 Additional comments (2)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
1838-1842: Add documentation for the accessibility check bypass in version 48 tests.Axe accessibility violations for version 48 (HeadingText color tests) are being logged as warnings instead of causing test failures. Add a TODO comment explaining:
- What specific accessibility violations exist in the HeadingText color scenarios
- Whether these are known limitations of the palette-based site colors
- The timeline for addressing these violations
Without documentation, future maintainers won't understand the rationale for bypassing these checks.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/atoms/heading.tsx (1)
130-136: Consider simplifying the normalize function.The normalize helper handles both
bg-andtext-prefixes, but onlycolor.bgColoris passed to it (line 100). Since BackgroundStyle.bgColor always uses thebg-prefix pattern, thetext-branch appears unnecessary.🔎 Simplified implementation
If
text-handling isn't needed for future extensibility:-const normalize = (token?: string) => - token?.startsWith("bg-") - ? token.substring(3) - : token?.startsWith("text-") - ? token.substring(5) - : token; +const normalize = (token?: string) => + token?.startsWith("bg-") ? token.substring(3) : token;Alternatively, if you want to keep both prefixes for future use, consider adding a comment explaining the intent.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (30)
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/[desktop] version 48 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 48 - HeadingText on light background.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/[mobile] version 48 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 48 - HeadingText on light background.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/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 48 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 48 - HeadingText on light background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (29)
packages/visual-editor/locales/cs/visual-editor.jsonpackages/visual-editor/locales/da/visual-editor.jsonpackages/visual-editor/locales/de/visual-editor.jsonpackages/visual-editor/locales/en-GB/visual-editor.jsonpackages/visual-editor/locales/en/visual-editor.jsonpackages/visual-editor/locales/es/visual-editor.jsonpackages/visual-editor/locales/et/visual-editor.jsonpackages/visual-editor/locales/fi/visual-editor.jsonpackages/visual-editor/locales/fr/visual-editor.jsonpackages/visual-editor/locales/hr/visual-editor.jsonpackages/visual-editor/locales/hu/visual-editor.jsonpackages/visual-editor/locales/it/visual-editor.jsonpackages/visual-editor/locales/ja/visual-editor.jsonpackages/visual-editor/locales/lt/visual-editor.jsonpackages/visual-editor/locales/lv/visual-editor.jsonpackages/visual-editor/locales/nb/visual-editor.jsonpackages/visual-editor/locales/nl/visual-editor.jsonpackages/visual-editor/locales/pl/visual-editor.jsonpackages/visual-editor/locales/pt/visual-editor.jsonpackages/visual-editor/locales/ro/visual-editor.jsonpackages/visual-editor/locales/sk/visual-editor.jsonpackages/visual-editor/locales/sv/visual-editor.jsonpackages/visual-editor/locales/tr/visual-editor.jsonpackages/visual-editor/locales/zh-TW/visual-editor.jsonpackages/visual-editor/locales/zh/visual-editor.jsonpackages/visual-editor/src/components/atoms/heading.tsxpackages/visual-editor/src/components/contentBlocks/HeadingText.tsxpackages/visual-editor/src/components/layoutBlocks/Grid.test.tsxpackages/visual-editor/src/utils/themeConfigOptions.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T16:36:32.810Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 958
File: packages/visual-editor/locales/ro/visual-editor.json:394-394
Timestamp: 2025-12-23T16:36:32.810Z
Learning: In Romanian locale files (ro), "Top" is an acceptable translation for directional terms like "top_direction" rather than the native Romanian "Sus". The maintainer benlife5 confirmed this is the preferred translation in packages/visual-editor/locales/ro/visual-editor.json.
Applied to files:
packages/visual-editor/locales/ro/visual-editor.jsonpackages/visual-editor/locales/cs/visual-editor.json
🧬 Code graph analysis (2)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
backgroundColors(50-125)
packages/visual-editor/src/components/atoms/heading.tsx (1)
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
BackgroundStyle(36-43)
🔇 Additional comments (7)
packages/visual-editor/locales/es/visual-editor.json (1)
608-608: LGTM! Localization additions for recommended color feature.The new translation keys for "recommendedColor" and "theme.colors.recommendedColorDescription" are properly structured and consistent across all locale files reviewed (es, sv, fi, de, it, ro, pt, en). These additions support the recommended color feature described in the PR objectives, providing accessibility-focused color contrast guidance.
Also applies to: 675-675
packages/visual-editor/locales/nb/visual-editor.json (1)
609-609: LGTM! Localization additions support new color feature.The new localization keys
recommendedColorandtheme.colors.recommendedColorDescriptionare properly structured and consistently added across all locale files (nb, da, et, sk, hu, ja, tr, cs). These translations support the new heading color selection feature described in the PR objectives, providing users with accessibility-focused color recommendations.Also applies to: 676-676
packages/visual-editor/locales/pl/visual-editor.json (1)
610-610: LGTM! Locale additions are consistent and well-structured.The additions of
recommendedColorandrecommendedColorDescriptionfollow the same pattern across all locale files in this PR, supporting the new accessibility-focused color feature for heading text.Also applies to: 677-677
packages/visual-editor/src/components/contentBlocks/HeadingText.tsx (1)
16-16: LGTM! Clean integration of the color styling feature.The HeadingText component correctly:
- Adds the optional color prop to its styles interface
- Passes the color through to the Heading component
- Exposes the color field in the UI using SITE_COLOR options
The implementation maintains backward compatibility by making color optional and not setting a default value.
Also applies to: 34-34, 80-80, 119-122
packages/visual-editor/src/utils/themeConfigOptions.ts (2)
163-192: Well-structured refactoring of color options.The two-group structure clearly separates:
- A "Recommended Color" with an accessible default (undefined value)
- "Site Colors" with explicit color choices from the theme
This aligns well with the BackgroundStyle-based color system and provides a clean UI organization. The filter logic correctly identifies color entries from backgroundColors.
163-192: The "dynamic default" description is misleading; clarify what dynamic behavior is actually implemented.The description at lines 167-168 states "Optimize color contrast for accessibility by using the dynamic default," which suggests runtime contrast optimization. However, the implementation simply sets the Default option's value to
undefined, allowing the Heading component to resolve a pre-configured CSS variable. This is not dynamic—it's static theme configuration.In accessibility contexts, "dynamic default" typically implies algorithmic color generation or runtime adaptation based on background contrast (e.g., Material Dynamic Color). The code lacks such logic;
isDarkBackgroundis available but unused in the Heading component.Revise the description to accurately reflect the behavior: "Use the default color setting, which resolves to a pre-configured CSS variable optimized for the theme."
Additionally, the filter
key.includes("color")at line 182 works correctly for current keys but lacks specificity. Usekey.startsWith("color")to prevent unintended matches if new color-related keys are added.packages/visual-editor/src/components/atoms/heading.tsx (1)
99-101: Thenormalizefunction correctly extracts color token names from Tailwind classes (e.g.,"bg-palette-secondary-dark"→"palette-secondary-dark"), and the generated CSS variable pattern--colors-{token}is consistent with usage throughout the codebase. This implementation is sound and does not require changes.
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/visual-editor/locales/hr/visual-editor.json (2)
614-614: LGTM! Structural addition is correct.The new root-level localization key is properly placed alphabetically and complements the existing plural form. JSON syntax is valid.
If not already done, consider having a native Croatian speaker verify the translation quality for "Preporučena boja" to ensure it reads naturally in context.
681-681: LGTM! Description key properly nested.The localization key is correctly placed within the
theme.colorssection and the translation conveys the accessibility/contrast optimization message. JSON structure is valid.If not already done, consider having a native Croatian speaker review the phrasing of "Optimizirajte kontrast boja za pristupačnost pomoću dinamičke zadane postavke" to ensure it's clear and natural for end users.
packages/visual-editor/locales/ja/visual-editor.json (1)
614-614: Consider verifying translation quality with a native speaker.While the translations appear semantically correct, it's good practice to have a native Japanese speaker review localization strings to ensure they are natural and idiomatic, especially for user-facing UI elements.
Also applies to: 681-681
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
163-192: Two-group restructure effectively separates dynamic default from explicit site colors.The logic correctly filters
backgroundColorsentries and maintains type compatibility withComboboxOptionGroup[], which supports multiple groups. Theundefinedcolor value for the Default option properly enables dynamic color behavior.Duplication confirmed: The filtering logic at lines 180–190 duplicates the pattern from
backgroundColorOptions(lines 149–159). Consider extracting a helper:+const buildColorOptions = (filterKey: string) => + Object.entries(backgroundColors) + .map(([key, { label, value }]) => { + if (key.includes(filterKey)) { + return { + label, + value, + color: value.bgColor, + }; + } + }) + .filter((o) => !!o); + export const siteColorOptions: ComboboxOptionGroup[] = [ { title: msg("recommendedColor", "Recommended Color"), description: msg( "theme.colors.recommendedColorDescription", "Optimize color contrast for accessibility by using the dynamic default." ), options: [ { label: msg("default", "Default"), value: undefined, color: undefined, }, ], }, { title: msg("siteColors", "Site Colors"), - options: Object.entries(backgroundColors) - .map(([key, { label, value }]) => { - if (key.includes("color")) { - return { - label, - value, - color: value.bgColor, - }; - } - }) - .filter((o) => !!o), + options: buildColorOptions("color"), }, ];i18n keys verified:
recommendedColor,theme.colors.recommendedColorDescription, andsiteColorsexist in all 21+ locale files. EnsureBasicSelectorand downstream consumers properly handle the two-group structure andundefinedcolor fallback.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
packages/visual-editor/locales/cs/visual-editor.jsonpackages/visual-editor/locales/da/visual-editor.jsonpackages/visual-editor/locales/de/visual-editor.jsonpackages/visual-editor/locales/en-GB/visual-editor.jsonpackages/visual-editor/locales/en/visual-editor.jsonpackages/visual-editor/locales/es/visual-editor.jsonpackages/visual-editor/locales/et/visual-editor.jsonpackages/visual-editor/locales/fi/visual-editor.jsonpackages/visual-editor/locales/fr/visual-editor.jsonpackages/visual-editor/locales/hr/visual-editor.jsonpackages/visual-editor/locales/hu/visual-editor.jsonpackages/visual-editor/locales/it/visual-editor.jsonpackages/visual-editor/locales/ja/visual-editor.jsonpackages/visual-editor/locales/lt/visual-editor.jsonpackages/visual-editor/locales/lv/visual-editor.jsonpackages/visual-editor/locales/nb/visual-editor.jsonpackages/visual-editor/locales/nl/visual-editor.jsonpackages/visual-editor/locales/pl/visual-editor.jsonpackages/visual-editor/locales/pt/visual-editor.jsonpackages/visual-editor/locales/ro/visual-editor.jsonpackages/visual-editor/locales/sk/visual-editor.jsonpackages/visual-editor/locales/sv/visual-editor.jsonpackages/visual-editor/locales/tr/visual-editor.jsonpackages/visual-editor/locales/zh-TW/visual-editor.jsonpackages/visual-editor/locales/zh/visual-editor.jsonpackages/visual-editor/src/components/layoutBlocks/Grid.test.tsxpackages/visual-editor/src/utils/themeConfigOptions.tsstarter/package.json
✅ Files skipped from review due to trivial changes (1)
- starter/package.json
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/es/visual-editor.json
- packages/visual-editor/locales/pl/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 (12)
packages/visual-editor/locales/fi/visual-editor.json (1)
614-614: LGTM! Localization keys properly added.The new localization keys for the recommended color feature are correctly placed and follow proper JSON structure. The alphabetical ordering is maintained, and the changes align with the PR objectives to add color options for heading text.
If the Finnish translations have not already been reviewed by your localization team, please verify their accuracy.
Also applies to: 681-681
packages/visual-editor/locales/sk/visual-editor.json (1)
615-615: LGTM! Localization keys added correctly.The new
recommendedColorandrecommendedColorDescriptionkeys are properly structured, syntactically valid, and alphabetically ordered. These additions align with the PR's objective to add color options for Heading text and enhance the Site Colors dropdown with recommended descriptions.If translation quality verification by a native Slovak speaker hasn't been performed yet, consider having these strings reviewed to ensure accuracy and natural phrasing.
Also applies to: 682-682
packages/visual-editor/locales/nb/visual-editor.json (1)
614-614: LGTM!The Norwegian translations for the new "Recommended Color" feature are correctly added. The JSON structure is valid, and the keys align with the PR objectives to add color options for heading text with accessibility-focused descriptions.
Also applies to: 681-681
packages/visual-editor/locales/ja/visual-editor.json (1)
614-614: LGTM! Localization keys properly structured.The new localization keys are correctly placed alphabetically and follow the existing JSON structure. The additions support the recommended color feature described in the PR objectives.
Also applies to: 681-681
packages/visual-editor/locales/da/visual-editor.json (1)
621-621: LGTM! Translation additions support the new recommended color feature.The Danish translations for the new recommended color keys are clear and align with the PR's objective to add color options for heading text with accessibility-focused defaults.
Also applies to: 688-688
packages/visual-editor/locales/en/visual-editor.json (1)
613-613: LGTM! English translations establish clear accessibility messaging.The English locale additions clearly communicate the recommended color feature's purpose: optimizing color contrast for accessibility. The "dynamic default" language appropriately describes the behavior mentioned in the PR objectives.
Also applies to: 680-680
packages/visual-editor/locales/lv/visual-editor.json (1)
614-614: LGTM! Latvian translations added consistently.The Latvian locale additions follow the same pattern as other locales, providing translations for the new recommended color feature keys.
Also applies to: 681-681
packages/visual-editor/locales/lt/visual-editor.json (1)
614-614: LGTM! Lithuanian translations added consistently.The Lithuanian locale additions maintain consistency with other locale files, providing appropriate translations for the recommended color feature.
Also applies to: 681-681
packages/visual-editor/locales/fr/visual-editor.json (1)
614-614: LGTM! French translations complete the locale expansion.The French locale additions are consistent with the pattern across all reviewed locale files, providing professional translations for the recommended color feature that emphasizes accessibility.
Also applies to: 681-681
packages/visual-editor/locales/pt/visual-editor.json (2)
614-614: LGTM! New localization keys added correctly.The additions of
recommendedColorandrecommendedColorDescriptionare properly formatted and alphabetically positioned. The translations are appropriate, and both keys are confirmed to be referenced in the codebase atpackages/visual-editor/src/utils/themeConfigOptions.ts.
681-681: Translation is accurate and key is properly integrated.The
recommendedColorDescriptionkey is correctly formatted and positioned withintheme.colors. The Portuguese translation accurately renders the English original, and the key is actively used in the feature code atpackages/visual-editor/src/utils/themeConfigOptions.ts. This is a consistent addition across 25+ locale files.packages/visual-editor/locales/nl/visual-editor.json (1)
614-614: Translations verified as accurate and appropriate.Both Dutch translations are correct and natural for native speakers:
- "Aanbevolen kleur" correctly translates "recommended color"
- "Optimaliseer het kleurcontrast voor toegankelijkheid door de dynamische standaard te gebruiken." accurately conveys the description and appropriately includes context about the dynamic default
The JSON structure, key placement, and translations align with the PR objectives. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
1828-1911: Consider using consistent ID naming for the light background test.The IDs in the light background test use
HeadingText-dark-bg-Xprefix, which is misleading since this test exercises light backgrounds. While this doesn't cause runtime issues (tests run in isolation), it reduces readability.🔎 Suggested ID naming fix
- id: "HeadingText-dark-bg-1", + id: "HeadingText-light-bg-1", ... - id: "HeadingText-dark-bg-2", + id: "HeadingText-light-bg-2", ... - id: "HeadingText-dark-bg-3", + id: "HeadingText-light-bg-3", ... - id: "HeadingText-dark-bg-4", + id: "HeadingText-light-bg-4", ... - id: "HeadingText-dark-bg-5", + id: "HeadingText-light-bg-5",
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
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/[desktop] version 50 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 50 - HeadingText on light background.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/[mobile] version 50 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 50 - HeadingText on light background.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/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 50 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 50 - HeadingText on light background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
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 (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (2)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (2)
1679-1801: LGTM!The dark background test case is well-structured with unique IDs, diverse heading levels, and a good mix of explicit color selections plus a default (no color) case to verify the PR objective that heading color respects user selection.
1985-1999: Verify that the accessibility bypass for version 50 is intentional.The accessibility checks are now bypassed for version 50 tests (matching the existing pattern for version 45). Results are logged via
console.warnrather than asserted.If this is intentional due to known accessibility issues with the new color feature, consider adding a TODO comment to track when these should be re-enabled, or document why the bypass is necessary.
...omponents/testing/screenshots/Grid/[desktop] version 48 - HeadingText on dark background.png
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 50 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 50 - HeadingText on light background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 50 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 50 - HeadingText on light background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 50 - HeadingText on dark background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 50 - HeadingText on light background.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (4)
packages/visual-editor/src/components/atoms/cta.tsxpackages/visual-editor/src/components/atoms/heading.tsxpackages/visual-editor/src/components/layoutBlocks/Grid.test.tsxpackages/visual-editor/src/utils/normalizeThemeColor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/components/atoms/heading.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
packages/visual-editor/src/utils/themeConfigOptions.ts (1)
backgroundColors(50-125)
packages/visual-editor/src/components/atoms/cta.tsx (1)
packages/visual-editor/src/utils/normalizeThemeColor.ts (1)
normalizeThemeColor(2-14)
⏰ 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 (4)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (2)
1679-1924: Well-structured test coverage for HeadingText color options.The two new test cases provide comprehensive coverage of HeadingText with different site colors on both dark and light backgrounds. All element IDs are unique, which successfully addresses the previous review concern about duplicate IDs.
1985-1985: Plan to address accessibility violations for version 50.The accessibility checks are being bypassed for version 50 (similar to version 45), logging warnings instead of failing tests. While this is common during feature development, ensure there's a plan to resolve these violations before the feature is considered complete.
Consider tracking the accessibility violations with a follow-up issue to ensure they're addressed in a future iteration.
Also applies to: 1996-1996
packages/visual-editor/src/components/atoms/cta.tsx (1)
17-17: LGTM! Clean refactor to shared utility.The migration from a local
normalizehelper to the sharednormalizeThemeColorutility centralizes color token handling and maintains the same behavior.Also applies to: 189-190
packages/visual-editor/src/utils/normalizeThemeColor.ts (1)
2-14: The function correctly handles all token patterns actually used in the codebase. AllbgColorandtextColorvalues throughout the application follow the simplebg-{color}ortext-{color}format (e.g.,bg-white,bg-palette-primary-light,text-black,text-white), and the function processes them as expected.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Hmm pnpm.lock-yaml shouldnt have changed?
Ticket - OPAQF-26
Screen.Recording.2025-12-29.at.3.13.21.PM.mov