feat: added link to header logo #943
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. |
WalkthroughAdded a new translatable Sequence Diagram(s)sequenceDiagram
participant Browser
participant ImageComp as Image Component
participant Data as Component Data
participant I18n as i18n Resolver
participant Validator as URL Validator
participant LinkComp as MaybeLink
Browser->>ImageComp: mount / render image block
ImageComp->>Data: read data.link (TranslatableString)
Data->>I18n: resolve localized link for current language & document context
I18n-->>ImageComp: resolvedUrl (string or undefined)
ImageComp->>Validator: validate resolvedUrl (regex)
alt valid URL
ImageComp->>LinkComp: render MaybeLink(href=resolvedUrl) wrapping <img>
else invalid or missing
ImageComp->>ImageComp: render <img> without active link (or use default "#")
end
LinkComp-->>Browser: DOM with image (wrapped or not) rendered
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (25)
🚧 Files skipped from review as they are similar to previous changes (18)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (7)
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: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
144-163: Consider extracting duplicated Link wrapper to reduce repetition.The mobile and desktop logo sections have identical
Linkwrappers. While functional, extracting this to a shared element would improve maintainability.+ const LogoLink = ( + <Link + href={resolvedLink} + eventName="clickedLogo" + aria-label={t("logoLink", "Logo Link")} + > + <slots.LogoSlot style={{ height: "auto", width: "auto" }} /> + </Link> + ); + return ( <> <div className="flex flex-col"> <PageSection maxWidth={parentValues?.maxWidth} verticalPadding={"header"} background={styles.backgroundColor} className="flex flex-row justify-between w-full items-center gap-8" > {/* Mobile logo */} - <div className="block md:hidden"> - <Link - href={resolvedLink} - eventName="clickedLogo" - aria-label={t("logoLink", "Logo Link")} - > - <slots.LogoSlot style={{ height: "auto", width: "auto" }} /> - </Link> - </div> + <div className="block md:hidden">{LogoLink}</div> {/* Desktop logo */} - <div className="hidden md:block"> - <Link - href={resolvedLink} - eventName="clickedLogo" - aria-label={t("logoLink", "Logo Link")} - > - <slots.LogoSlot style={{ height: "auto", width: "auto" }} /> - </Link> - </div> + <div className="hidden md:block">{LogoLink}</div>
📜 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/header/PrimaryHeaderSlot.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
packages/visual-editor/src/types/types.ts (1)
TranslatableString(198-198)
⏰ 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 (22)
packages/visual-editor/locales/hr/visual-editor.json (1)
275-275: LGTM!Both translation keys are correctly placed (alphabetically ordered) and the JSON formatting is valid. The Croatian translation "Link na logotip" aligns with the pattern used for similar keys in the file (e.g., "Facebook veza", "Pinterest veza") and properly supports the new Logo Link feature in the Primary Header component.
Also applies to: 534-534
packages/visual-editor/locales/sv/visual-editor.json (1)
276-276: Localization additions are syntactically correct and complete across all locales.Both
logoLinkentries in the Swedish file are properly formatted, maintain alphabetical ordering, and integrate well with surrounding keys. The Swedish translation "Logotyp länk" is appropriate. Verification confirms that all 25 locale files have received correspondinglogoLinkentries with language-appropriate translations, with consistent placement at both the fields and top-level sections.packages/visual-editor/locales/et/visual-editor.json (1)
275-275: ✓ Approved: Consistent localization additions for logoLink featureBoth logoLink entries are properly positioned (within fields.logo and top-level logo sections) and follow the established JSON structure. The Estonian translation "Logo link" is consistent with the pattern used across other locales in this PR.
Also applies to: 534-534
packages/visual-editor/locales/de/visual-editor.json (1)
280-280: ✓ Approved: Correct German translation with compound word formattingThe logoLink entries use appropriate German formatting ("Logo-Link" with hyphen). Both locations are correctly placed and maintain JSON structure integrity.
Also applies to: 539-539
packages/visual-editor/locales/it/visual-editor.json (1)
275-275: ✓ Approved: Natural Italian translation placementThe Italian translation "Collegamento al logo" is properly positioned and naturally phrased for Italian speakers. Both logoLink entries maintain consistency across the JSON structure.
Also applies to: 534-534
packages/visual-editor/locales/lv/visual-editor.json (1)
275-275: ✓ Approved: Consistent Latvian translation with correct grammatical formBoth logoLink entries are correctly placed with proper Latvian morphology ("Logotipa saite"). JSON structure and spacing are maintained correctly.
Also applies to: 534-534
packages/visual-editor/locales/es/visual-editor.json (1)
274-274: ✓ Approved: Proper Spanish translation with correct grammarThe Spanish translation "Enlace del logotipo" correctly uses the definite article construction and is naturally phrased. Both entries are properly positioned within the JSON structure.
Also applies to: 533-533
packages/visual-editor/locales/zh-TW/visual-editor.json (1)
275-275: ✓ Approved: Correct Traditional Chinese translationBoth logoLink entries use appropriate Traditional Chinese characters ("標誌鏈接"). Placement is consistent across both locations in the JSON structure.
Also applies to: 534-534
packages/visual-editor/locales/ro/visual-editor.json (1)
275-275: ✓ Approved: Clear Romanian translation placementBoth logoLink entries are correctly positioned with straightforward, natural Romanian phrasing ("Link logo"). JSON structure integrity maintained.
Also applies to: 534-534
packages/visual-editor/locales/en-GB/visual-editor.json (1)
274-274: ✓ Approved: Consistent English translation across both locationsBoth logoLink entries are properly positioned with clear, natural English phrasing ("Logo Link"). JSON structure and formatting are maintained correctly.
Also applies to: 533-533
packages/visual-editor/locales/pt/visual-editor.json (1)
275-275: LGTM!The Portuguese translation for
logoLinkis correctly added in both required locations (withinfieldsand at the top-level), consistent with the pattern across other locale files.Also applies to: 534-534
packages/visual-editor/locales/da/visual-editor.json (1)
282-282: LGTM!The Danish translation for
logoLinkis correctly added in both required locations.Also applies to: 541-541
packages/visual-editor/locales/nb/visual-editor.json (1)
275-275: LGTM!The Norwegian Bokmål translation for
logoLinkis correctly added in both required locations.Also applies to: 534-534
packages/visual-editor/locales/zh/visual-editor.json (1)
275-275: LGTM!The Simplified Chinese translation for
logoLinkis correctly added in both required locations.Also applies to: 534-534
packages/visual-editor/locales/fr/visual-editor.json (1)
274-274: LGTM!The French translation for
logoLinkis correctly added in both required locations.Also applies to: 533-533
packages/visual-editor/locales/pl/visual-editor.json (1)
276-276: LGTM!The Polish translation for
logoLinkis correctly added in both required locations.Also applies to: 535-535
packages/visual-editor/locales/en/visual-editor.json (1)
274-274: LGTM!The English translation for
logoLinkis correctly added in both required locations, serving as the reference for other locales.Also applies to: 533-533
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (5)
17-25: LGTM on new imports and constants.The imports and constants are well-organized. The
LINK_REGEX_VALIDATIONpattern appropriately covers http/https URLs, relative paths (starting with/), and anchor links (starting with#).
27-30: Clean interface extension.The
dataproperty withlogoLink: TranslatableStringproperly supports localized link values, aligning with the existing pattern for translatable content in this codebase.
56-64: Well-structured field declaration.The
logoLinkfield is correctly nested underdatausing thetranslatableStringtype, consistent with how other translatable fields are declared.
116-121: Solid link resolution with validation and fallback.The logic correctly:
- Resolves the link using
resolveComponentDatawith the current language- Validates against the URL/path/anchor pattern
- Falls back to
DEFAULT_LOGO_LINKfor invalid valuesThis defensive approach prevents broken or malicious links from being rendered.
259-262: ThehasLocalizedValuefield is correctly set as a string"true". The type definition inpackages/visual-editor/src/types/types.tsexplicitly definesLocalizedValueswithhasLocalizedValue: "true"as a string literal type (not a boolean), andTranslatableStringis correctly typed asstring | LocalizedValues. This matches the intended design and all usages throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (2)
17-18: Logo link validation and default anchor behaviorThe regex currently only allows
http(s)URLs, root-relative paths (/…), and hash anchors (#…). This excludes other common schemes likemailto:,tel:, protocol-relative URLs (//example.com), or relative paths without a leading slash, which would then silently fall back to"#". That may surprise editors who enter a seemingly valid link and get a no-op anchor.If the intent is to support a broader set of schemes, consider extending the regex and/or making the fallback behavior more explicit (e.g., leaving the href as-is or rendering a non-clickable logo when invalid), otherwise documenting this constraint in the field description would help avoid confusion.
Also applies to: 22-25
98-105: Robust link resolution with minor readability improvement opportunitiesThe pattern of resolving
data.logoLinkviaresolveComponentDataandi18n.language, validating it, and centralizing the clickable logo in aLogoLinkvariable is solid and keeps the render JSX clean.Two small optional improvements:
- Avoid calling
.trim()twice by normalizing once before validation and reuse.- If you ever need different behavior between mobile and desktop logos (e.g., different
aria-labelor tracking), you may want to switchLogoLinkfrom a JSX variable to a small component that accepts overrides for those cases.Neither is blocking; the current implementation is correct.
Also applies to: 116-121, 123-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
packages/visual-editor/src/types/types.ts (1)
TranslatableString(198-198)
⏰ 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 (3)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (3)
28-30: Props shape and field config alignment look consistentAdding
data.logoLink: TranslatableStringon the props type and wiring it up as atranslatableStringfield underdatainprimaryHeaderSlotFieldsis consistent with how other translatable content is modeled in this file. This should integrate cleanly with the editor UI and default props.Also applies to: 57-65
155-158: Shared LogoLink usage for mobile and desktop is cleanReusing the same
LogoLinkJSX for both mobile (block md:hidden) and desktop (hidden md:block) containers keeps behavior consistent and avoids duplication compared to wrappingslots.LogoSlotseparately in each breakpoint.
254-256: Default logoLink value matches new data fieldProviding a default
data.logoLinkpointing atDEFAULT_LOGO_LINKensuresresolveComponentDatahas a well-formed value even before editors touch the field, which should prevent undefined access at runtime. This aligns with how default translatable values are set elsewhere in the file.
asanehisa
left a comment
asanehisa
left a comment
There was a problem hiding this comment.
Hmmm 1) tests failing, 2) I think we'd add it as a field on the logoSlot
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (2)
98-105: Logo link resolution & rendering are generally solid; consider a couple of small refinements
- Using
useTranslation’si18n.languagewithresolveComponentDatato obtain a per-locale URL, and then validating/normalizing viaLINK_REGEX_VALIDATIONandtrim(), is a safe flow with a clear fallback toDEFAULT_LOGO_LINK. This avoids brokenhrefs while still honoring localized config.- Wrapping the logo slot in a
Linkwitharia-label={t("logoLink", "Logo Link")}is good for keyboard accessibility and screen readers, and leaving the logo rendering itself toslots.LogoSlotavoids layout regressions.- Reusing
LogoLinkfor both mobile and desktop containers keeps markup DRY and maintains existing flex layout.Two minor polish ideas:
- The
LogoLinkconstant is a ReactNode, not a component. Renaming tologoLink(lowercase) could make that intent clearer, though this is stylistic.- If you expect non-HTTP schemes in the future (e.g.,
mailto:,tel:), you might eventually want to relax or centralizeLINK_REGEX_VALIDATIONto keep link rules consistent across components.None of these are blockers.
Also applies to: 116-135, 158-160
257-259: Defaultdata.logoLinkis reasonable; confirmhasLocalizedValueshape matches conventionsProviding a default
data.logoLinkwith{ en: DEFAULT_LOGO_LINK, hasLocalizedValue: "true" }ensures the header logo is always clickable, even without editor configuration.Small detail to confirm: other
TranslatableStringdefaults in this codebase sometimes use a string"true"forhasLocalizedValue; if there’s any inconsistency (boolean vs string) elsewhere, it might be worth standardizing in a follow‑up, but this matches patterns commonly used for translatable labels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
packages/visual-editor/src/types/types.ts (1)
TranslatableString(198-198)
⏰ 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 (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
17-18: Imports and URL validation constants look consistentUsing
TranslatableStringfor the logo URL and pullingLinkfrom@yext/pages-componentsalign with the rest of the file’s patterns. TheDEFAULT_LOGO_LINKandLINK_REGEX_VALIDATIONprovide a safe fallback and basic sanitation without over-complicating things. No issues here.Also applies to: 22-25
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
28-30: Acknowledge past review: Test data needs updating.The past review comment correctly identifies that test files need to include the new required
datafield. Please ensureExpandedHeader.test.tsxis updated as indicated in the previous feedback.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
17-17: LGTM: Imports and constants are well-structured.The imports are appropriate, and the constants provide clear defaults and validation. The regex pattern covers the main URL/path formats needed.
Optional: Consider stricter regex for empty paths.
The current regex allows
#alone (which matches your default) but also permits bare/or#with no following characters. If you want to enforce that relative paths or hashes have meaningful content, you could tighten the pattern:-const LINK_REGEX_VALIDATION = /^(https?:\/\/[^\s]+|\/[^\s]*|#[^\s]*)$/; +const LINK_REGEX_VALIDATION = /^(https?:\/\/[^\s]+|\/[^\s]+|#[^\s]+)$/;This would require at least one character after
/or#, though given your default is#, the current pattern is reasonable.Also applies to: 22-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
packages/visual-editor/src/types/types.ts (1)
TranslatableString(198-198)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (5)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (5)
98-98: LGTM: Props destructuring and i18n access are correct.The destructuring of
dataand the addition ofi18nto theuseTranslationhook are necessary and properly implemented for the logo link feature.Also applies to: 105-105
116-134: LGTM: Logo link resolution and component implementation are solid.The link resolution logic correctly:
- Resolves translatable strings using the current locale
- Validates URLs with regex
- Trims whitespace
- Falls back to a safe default on invalid input
The
LogoLinkcomponent properly wraps the logo with a clickable link, includes analytics tracking (eventName), and provides accessibility viaaria-label.
158-158: LGTM: Logo link consistently applied across mobile and desktop views.The
LogoLinkis correctly used in both the mobile and desktop rendering paths, ensuring a consistent clickable logo experience across all breakpoints.Also applies to: 160-160
257-259: LGTM: Default props properly initialized.The default
datafield is correctly structured with a safe fallback link (#) and proper localization metadata. This ensures new slot instances have valid initial state.
57-64: The field definitions are correct and properly support internationalization. The translation keys"fields.data"and"fields.logoLink"are already defined in all locale files across all supported languages (English, French, German, Spanish, Japanese, Chinese, and others). No action is required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (2)
29-30: Verify URL validation coverage for your use case.The
LINK_REGEX_VALIDATIONpattern only supports:
http://andhttps://URLs- Absolute paths starting with
/- Hash fragments (
#)It does not support:
- Relative URLs without leading
/(e.g.,../page.html,page.html)mailto:ortel:links- Other URL protocols
If these link types are needed, the regex should be expanded. Otherwise, consider adding user-facing validation messages to clarify supported formats.
Minor: Consider renaming
LINK_REGEX_VALIDATIONtoURL_VALIDATION_REGEXto follow typical naming conventions (subject before type).
125-136: Consider providing feedback for invalid URLs.The link validation silently falls back to
"#"when the URL is invalid. In edit mode (puck.isEditing), consider displaying a validation message to help users understand why their link isn't working.The resolution logic itself is well-structured and handles missing/undefined links appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
packages/visual-editor/locales/cs/visual-editor.json(1 hunks)packages/visual-editor/locales/da/visual-editor.json(1 hunks)packages/visual-editor/locales/de/visual-editor.json(1 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(1 hunks)packages/visual-editor/locales/en/visual-editor.json(1 hunks)packages/visual-editor/locales/es/visual-editor.json(1 hunks)packages/visual-editor/locales/et/visual-editor.json(1 hunks)packages/visual-editor/locales/fi/visual-editor.json(1 hunks)packages/visual-editor/locales/hr/visual-editor.json(1 hunks)packages/visual-editor/locales/hu/visual-editor.json(1 hunks)packages/visual-editor/locales/it/visual-editor.json(1 hunks)packages/visual-editor/locales/ja/visual-editor.json(1 hunks)packages/visual-editor/locales/lt/visual-editor.json(1 hunks)packages/visual-editor/locales/lv/visual-editor.json(1 hunks)packages/visual-editor/locales/nb/visual-editor.json(1 hunks)packages/visual-editor/locales/nl/visual-editor.json(1 hunks)packages/visual-editor/locales/pl/visual-editor.json(1 hunks)packages/visual-editor/locales/pt/visual-editor.json(1 hunks)packages/visual-editor/locales/ro/visual-editor.json(1 hunks)packages/visual-editor/locales/sk/visual-editor.json(1 hunks)packages/visual-editor/locales/sv/visual-editor.json(1 hunks)packages/visual-editor/locales/tr/visual-editor.json(1 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(1 hunks)packages/visual-editor/locales/zh/visual-editor.json(1 hunks)packages/visual-editor/src/components/contentBlocks/image/Image.tsx(6 hunks)packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx(3 hunks)packages/visual-editor/src/docs/components.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/visual-editor/src/docs/components.md
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/visual-editor/locales/nl/visual-editor.json
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/hr/visual-editor.json
- packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/da/visual-editor.json
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (3)
packages/visual-editor/src/types/images.ts (1)
AssetImageType(57-63)packages/visual-editor/src/types/types.ts (1)
TranslatableString(198-198)packages/visual-editor/src/components/atoms/image.tsx (1)
Image(25-87)
⏰ 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 (9)
packages/visual-editor/locales/de/visual-editor.json (1)
280-280: Translation addition is correctly placed and formatted.The new
logoLinkfield label with the German translation "Logo-Link" is appropriately positioned within thefields.logogroup and follows the established localization pattern. The translation is semantically correct.packages/visual-editor/locales/es/visual-editor.json (1)
274-274: LGTM! Translation addition looks correct.The new "logoLink" key with Spanish translation "Enlace del logotipo" is correctly positioned and formatted.
packages/visual-editor/locales/sv/visual-editor.json (1)
276-276: LGTM! Swedish translation is correct.The "logoLink" key with value "Logotyp länk" is properly formatted and positioned.
packages/visual-editor/locales/cs/visual-editor.json (1)
282-282: LGTM! Czech translation is correct.The "logoLink" key with value "Odkaz na logo" is properly added.
packages/visual-editor/locales/ro/visual-editor.json (1)
275-275: LGTM! Romanian translation is correct.The "logoLink" key with value "Link logo" is properly added.
packages/visual-editor/locales/en/visual-editor.json (1)
274-274: LGTM! Base English translation is correct.The "logoLink" key is properly added to the fields object.
packages/visual-editor/locales/lt/visual-editor.json (1)
275-275: LGTM! Lithuanian translation is correct.The "logoLink" key with value "Logotipo nuoroda" is properly added.
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (2)
36-36: LGTM! Optional link field is well-designed.The optional
link?: TranslatableStringfield properly supports internationalization and is correctly integrated into the field definitions.Also applies to: 69-71
196-214: LGTM! Default props are consistent.The default link value with
hasLocalizedValue: "true"is consistent with the resolution logic and properly structured.
packages/visual-editor/src/components/contentBlocks/image/Image.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (1)
175-190: Aria-label remains too generic for accessibility.The
aria-labelat line 179 still uses a generic translation "Clicked on Image" which provides minimal context for screen reader users. As noted in the previous review, consider:
- Using the image's alt text as the link label when available
- Providing a field for custom link descriptions
- Using a more descriptive default like "View image destination" or "Follow image link"
The rest of the Link wrapper implementation correctly disables pointer events for placeholder links and includes appropriate analytics tracking.
Based on learnings from previous review feedback.
🧹 Nitpick comments (2)
packages/visual-editor/locales/fr/visual-editor.json (1)
4-4: Consider present tense for better accessibility.The translation "J'ai cliqué sur l'image" uses past tense, which is less intuitive for
aria-labelattributes. Screen reader users typically expect present tense (e.g., "Cliquer sur l'image") or imperative mood labels that describe the action available. This applies to all locale files in this PR where "ClickedOnImage" uses past tense.Note: This same pattern appears in all locale additions (lt, et, hr, pt, and others in the PR).
Also applies to: 275-275, 534-534
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (1)
29-30: Consider more robust URL validation.The current
LINK_REGEX_VALIDATIONpattern provides basic validation but could be enhanced:
- It accepts any characters after
http://orhttps://(e.g.,http://not a valid urlwould pass)- Consider using a more robust URL validation library or a stricter pattern
- Alternatively, document that this intentionally permits loose validation for flexibility
Current implementation is functional but may allow malformed URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
packages/visual-editor/src/components/testing/screenshots/EventSection/[desktop] version 0 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[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/EventSection/[desktop] version 15 props with missing ctaType.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[desktop] version 26 props with constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[desktop] version 7 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[desktop] version 7 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[mobile] version 0 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[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/EventSection/[mobile] version 15 props with missing ctaType.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[mobile] version 7 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[mobile] version 7 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[tablet] version 0 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[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/EventSection/[tablet] version 15 props with missing ctaType.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[tablet] version 7 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[tablet] version 7 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[desktop] default props with coordinate - with api key.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(3 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/image/Image.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/es/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/nl/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/en/visual-editor.json
- packages/visual-editor/locales/da/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 (11)
packages/visual-editor/locales/de/visual-editor.json (1)
4-4: Localization additions look good. Both new keys (ClickedOnImageandlogoLink) are correctly added with appropriate German translations, and the JSON structure remains valid.Also applies to: 281-281
packages/visual-editor/locales/zh/visual-editor.json (1)
4-4: Consistent localization. Chinese translations for both keys are appropriately added with correct JSON formatting and placement.Also applies to: 276-276
packages/visual-editor/locales/it/visual-editor.json (1)
4-4: Italian translations added correctly. Both new locale keys are properly integrated with appropriate Italian translations and valid JSON syntax.Also applies to: 276-276
packages/visual-editor/locales/nb/visual-editor.json (1)
4-4: Norwegian Bokmål translations integrated successfully. Both keys are correctly positioned with semantically appropriate translations and valid JSON structure.Also applies to: 276-276
packages/visual-editor/locales/sk/visual-editor.json (1)
4-4: Slovak localization is complete. Both new translation keys are properly added with correct Slovak phrasing and valid JSON syntax.Also applies to: 277-277
packages/visual-editor/locales/ro/visual-editor.json (1)
4-4: Romanian translations are correctly placed. Both new locale keys are integrated with appropriate Romanian translations and maintain valid JSON formatting.Also applies to: 276-276
packages/visual-editor/locales/hu/visual-editor.json (1)
4-4: Hungarian localization looks good. Both new translation keys are properly added with appropriate Hungarian phrasing and valid JSON structure throughout.Also applies to: 276-276
packages/visual-editor/locales/sv/visual-editor.json (1)
4-4: Swedish translations are correctly integrated. Both new locale keys are added with semantically appropriate Swedish translations and proper JSON formatting.Also applies to: 277-277
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (3)
36-36: LGTM! Clean additive API change.The optional
linkfield addition is:
- Backward compatible (optional field)
- Properly typed as
TranslatableString- Correctly configured in the field definitions with appropriate i18n support
Also applies to: 69-71
125-136: Solid link resolution and validation logic.The implementation correctly:
- Resolves the link value from translatable string data
- Validates against the regex pattern and falls back to a safe default
- Tracks placeholder state to disable interactions for the default "#" link
This defensive approach prevents broken or malformed links from being rendered.
207-207: LGTM! Proper default props initialization.The default link value correctly initializes the optional field with:
- A safe placeholder value (
DEFAULT_LINK)- Proper
TranslatableStringstructure- Consistent typing with
hasLocalizedValue: "true" as const
|
@asanehisa , moved the logo link to |
packages/visual-editor/src/components/contentBlocks/image/Image.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/contentBlocks/image/Image.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/contentBlocks/image/Image.tsx
Outdated
Show resolved
Hide resolved
…d resolve fields for Header Logo only.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (1)
30-31: Clarify default"#"behavior and scope of image linking (header-only vs all images).Right now the combination of:
DEFAULT_LINK = "#",imageDefaultProps.data.linkdefaulting toDEFAULT_LINK, andresolvedLinkalways falling back toDEFAULT_LINKon missing/invalid inputmeans that every
ImageWrapperinstance gets a non-emptyhref(at least"#"), and the image is always rendered insideMaybeLink, even when:
- the link field is hidden outside
PrimaryHeaderSlotviaresolveFields, or- the editor leaves the Logo Link blank or types an invalid URL.
This makes generic images effectively clickable
"#"links and can hide misconfigurations (a bad URL silently degrades to"#"instead of becoming “no link”).If the intent is:
- To only have clickable behavior for the header logo by default, and
- To treat missing/invalid values as “no link”,
then it may be cleaner to:
- Avoid a global
DEFAULT_LINKin the base image defaults, and instead only set a default link (e.g."/") from the PrimaryHeaderSlot config; and- Let
resolvedLinkbeundefinedwhen there is no valid URL, e.g.:const inputLink = data.link && resolveComponentData(data.link, i18n.language, streamDocument); const resolvedLink = typeof inputLink === "string" && LINK_REGEX_VALIDATION.test(inputLink.trim()) ? inputLink.trim() : undefined;With that, non-header images stay unlinked by default, and invalid URLs don’t silently turn into
"#". AssumingMaybeLinkomits the underlying<Link>whenhrefis undefined/falsy, the runtime behavior would still match the desired “only link when there’s a valid URL” semantics.Can you double-check that this matches the product intent (header-only linking vs global), and that
MaybeLinkindeed skips rendering a link wrapper whenhrefis undefined?Also applies to: 37-38, 70-72, 126-137, 196-208, 221-230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[mobile] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (26)
packages/visual-editor/locales/cs/visual-editor.json(1 hunks)packages/visual-editor/locales/da/visual-editor.json(1 hunks)packages/visual-editor/locales/de/visual-editor.json(1 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(1 hunks)packages/visual-editor/locales/en/visual-editor.json(1 hunks)packages/visual-editor/locales/es/visual-editor.json(1 hunks)packages/visual-editor/locales/et/visual-editor.json(1 hunks)packages/visual-editor/locales/fi/visual-editor.json(1 hunks)packages/visual-editor/locales/fr/visual-editor.json(2 hunks)packages/visual-editor/locales/hr/visual-editor.json(1 hunks)packages/visual-editor/locales/hu/visual-editor.json(1 hunks)packages/visual-editor/locales/it/visual-editor.json(1 hunks)packages/visual-editor/locales/ja/visual-editor.json(1 hunks)packages/visual-editor/locales/lt/visual-editor.json(1 hunks)packages/visual-editor/locales/lv/visual-editor.json(1 hunks)packages/visual-editor/locales/nb/visual-editor.json(1 hunks)packages/visual-editor/locales/nl/visual-editor.json(1 hunks)packages/visual-editor/locales/pl/visual-editor.json(1 hunks)packages/visual-editor/locales/pt/visual-editor.json(1 hunks)packages/visual-editor/locales/ro/visual-editor.json(1 hunks)packages/visual-editor/locales/sk/visual-editor.json(1 hunks)packages/visual-editor/locales/sv/visual-editor.json(1 hunks)packages/visual-editor/locales/tr/visual-editor.json(1 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(1 hunks)packages/visual-editor/locales/zh/visual-editor.json(1 hunks)packages/visual-editor/src/components/contentBlocks/image/Image.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/hr/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/en/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/lt/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T19:26:15.855Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 942
File: packages/visual-editor/src/utils/schema/schemaBlocks.ts:29-35
Timestamp: 2025-12-12T19:26:15.855Z
Learning: In packages/visual-editor/src/utils/schema/schemaBlocks.ts, when siteDomain is missing (non-live/preview environments), schema id fields should use relative URLs rather than bare fragments, since Google will only crawl production sites where siteDomain is populated.
Applied to files:
packages/visual-editor/src/components/contentBlocks/image/Image.tsx
🔇 Additional comments (9)
packages/visual-editor/locales/fr/visual-editor.json (1)
274-274: Consistent and correct logoLink translations across French locale.The French locale correctly adds "logoLink": "Lien vers le logo" in both the fields object (line 274) and at root level (line 533), following the PR's intent to support logo linking in the Primary Header. Both translations are semantically accurate.
Also applies to: 533-533
packages/visual-editor/locales/da/visual-editor.json (1)
282-282: Correct Danish logoLink translation.The Danish locale adds "logoLink": "Logo link" in the correct position within the fields object. Translation is accurate and follows the consistent pattern.
packages/visual-editor/locales/cs/visual-editor.json (1)
282-282: Correct Czech logoLink translation.The Czech locale adds "logoLink": "Odkaz na logo" in the correct position within the fields object. Translation is accurate and maintains the expected pattern.
packages/visual-editor/locales/es/visual-editor.json (1)
274-274: Correct Spanish logoLink translation.The Spanish locale adds "logoLink": "Enlace del logotipo" in the correct position within the fields object. Translation is accurate and follows the consistent pattern.
packages/visual-editor/locales/sk/visual-editor.json (1)
276-276: Correct Slovak logoLink translation.The Slovak locale adds "logoLink": "Odkaz na logo" in the correct position within the fields object. Translation is accurate and maintains consistency.
packages/visual-editor/locales/nl/visual-editor.json (1)
275-275: Correct Dutch logoLink translation.The Dutch locale adds "logoLink": "Logo-link" in the correct position within the fields object. Translation is accurate and follows the consistent pattern.
packages/visual-editor/locales/ja/visual-editor.json (1)
275-275: Correct Japanese logoLink translation.The Japanese locale adds "logoLink": "ロゴリンク" in the correct position within the fields object. Translation appropriately uses katakana for technical terms, maintaining consistency with Japanese localization conventions.
packages/visual-editor/locales/it/visual-editor.json (1)
275-275: Correct Italian logoLink translation.The Italian locale adds "logoLink": "Collegamento al logo" in the correct position within the fields object. Translation is accurate and follows the consistent pattern across all locales.
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (1)
175-190: MaybeLink wrapper and analytics wiring look solid.The
MaybeLinkusage here (className,eventName="logoLink",alwaysHideCaret, and passing through the existingImageprops) cleanly adds clickable logo behavior and analytics without changing the image rendering path. No issues from my side on this block.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (1)
205-205: Use a more descriptive placeholder alt text.The
alternateText: "Test"appears to be a testing artifact. Consider using a more descriptive value such as"Placeholder image"or"Sample image"to better represent the placeholder content.- alternateText: "Test", + alternateText: "Placeholder image",
📜 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(3 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/image/Image.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/visual-editor/locales/nl/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
- packages/visual-editor/locales/en/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/fr/visual-editor.json
- packages/visual-editor/locales/en-GB/visual-editor.json
- packages/visual-editor/locales/zh-TW/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (7)
packages/visual-editor/src/components/contentBlocks/index.ts (1)
ImageWrapperProps(10-10)packages/visual-editor/src/types/images.ts (1)
AssetImageType(57-63)packages/visual-editor/src/types/types.ts (1)
TranslatableString(198-198)packages/visual-editor/scripts/generateFontRegistry.js (1)
data(42-42)packages/visual-editor/src/components/atoms/index.ts (2)
MaybeLink(12-12)Image(7-7)packages/visual-editor/src/components/atoms/image.tsx (1)
Image(25-87)packages/visual-editor/src/editor/index.ts (1)
resolveDataFromParent(28-28)
⏰ 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 (8)
packages/visual-editor/locales/zh/visual-editor.json (1)
275-276: Locale additions look good and consistent across all provided files.The
logoLinkandgoToHomepagetranslation keys have been added in semantically appropriate positions (logoLink near the existing logo field, goToHomepage near getDirections at root level) with valid translations across all 8 locales provided for review (zh, es, da, fi, sv, lt, hr, tr). JSON structure is valid, and the pattern is uniform.Note: This review covers localization additions only. The actual implementation that consumes these keys (ImageWrapper link prop wiring, PrimaryHeaderSlot refactoring) is not shown in the provided files, so full integration verification should be confirmed separately against the implementation code.
Also applies to: 489-489
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (7)
7-27: LGTM!The import additions are appropriate and all are utilized within the component for the link functionality.
30-31: LGTM!The default link and validation regex are well-defined. The regex pattern appropriately covers HTTP(S) URLs, absolute paths, and anchor links.
37-37: LGTM!The optional
linkfield is properly typed asTranslatableStringto support internationalization.
70-72: LGTM!The link field is properly configured with the correct type and follows the existing field definition pattern.
209-209: LGTM!The default link value is consistent with the resolution logic. Note that
hasLocalizedValue: "true" as constuses a string literal rather than a boolean, which appears to match the expected type signature.
223-235: LGTM!The conditional field visibility properly scopes the link field to only show for
PrimaryHeaderSlot, which addresses the concern from @benlife5 about limiting this feature to the header logo specifically.Based on learnings, this change directly addresses the previous review feedback about ensuring the link field appears only for the header logo.
126-136: The default link behavior creates unwanted link wrappers for all images.When
data.linkis undefined, the code defaults to#and passes it toMaybeLink. Since#is a truthy string,MaybeLinkrenders a CTA link even when the user didn't intend to add a link. Consider either conditionally applying the link only when explicitly provided by the user, or adding explicit logic to prevent rendering when the default is used.
packages/visual-editor/src/components/contentBlocks/image/Image.tsx
Outdated
Show resolved
Hide resolved
...components/testing/screenshots/ProductSection/[desktop] default props with document data.png
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (3)
30-31: Consider documenting link validation constraints.The regex restricts links to
http(s)://, absolute paths (/...), and hash fragments (#...). This excludes relative paths,mailto:,tel:, and other protocols. For the logo link use case (scoped to PrimaryHeaderSlot), these constraints are reasonable, but consider adding a comment explaining the validation rules for future maintainability.
126-137: Consider caching the trimmed value.The link resolution logic is correct, but
inputLink.trim()is called twice (lines 134, 136). Consider storing the trimmed value once for minor efficiency:+ const trimmedLink = typeof inputLink === "string" ? inputLink.trim() : ""; const resolvedLink = - typeof inputLink === "string" && - LINK_REGEX_VALIDATION.test(inputLink.trim()) && - inputLink.trim() !== DEFAULT_LINK - ? inputLink.trim() + trimmedLink && + LINK_REGEX_VALIDATION.test(trimmedLink) && + trimmedLink !== DEFAULT_LINK + ? trimmedLink : undefined;
176-191: Accessibility correctly implemented; consider making eventName configurable.The link wrapper correctly omits
aria-label, allowing the image's alt text to serve as the accessible name—excellent adherence to the learnings!However, the hardcoded
eventName="logoLink"(line 178) couples this genericImageWrappercomponent to a specific use case. While the link field is currently scoped toPrimaryHeaderSlot(lines 229-231), consider making the event name configurable via props or deriving it fromparams.parent.typefor better reusability:eventName={parentType === "PrimaryHeaderSlot" ? "logoLink" : "imageLink"}Based on learnings, the absence of aria-label is intentional and correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[desktop] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[mobile] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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 packages/visual-editor/src/components/contentBlocks/image/Image.tsx, when an Image is wrapped in a MaybeLink, do not set an aria-label on the link. Rely on the image's alt text as the accessible name for the link. Do not override with aria-label to preserve accessibility semantics. If the image lacks alt text, consider adding meaningful alt on the image itself rather than the link.
Applied to files:
packages/visual-editor/src/components/contentBlocks/image/Image.tsx
🔇 Additional comments (3)
packages/visual-editor/src/components/contentBlocks/image/Image.tsx (3)
1-27: LGTM!The new imports support the link functionality correctly:
MaybeLinkfor conditional wrapping,TranslatableStringfor the link field type, anduseTranslation/useDocumentfor internationalized link resolution.
37-37: LGTM!The
linkfield is properly defined as an optionalTranslatableString, includes a sensible default that gets filtered out during resolution, and uses appropriate localization keys.Also applies to: 70-72, 208-208
222-234: The resolveFields logic is correct as written.The
hideWidthPropandparentType === "PrimaryHeaderSlot"conditions are mutually exclusive in the codebase.hideWidthPropis only used in TeamCard and EventCard contexts, never in PrimaryHeaderSlot. The current implementation properly handles both cases:
- Images with
hideWidthPropin non-header contexts hide the width field- Images in PrimaryHeaderSlot (without
hideWidthProp) allow the link field to remain visibleNo refactor is needed.
Likely an incorrect or invalid review comment.
Since there were a lot of conflicts as the older [PR](#925) was before the slots, creating this off the latest main. Added a `Data` prop under `Primary Header.` with `Logo Link`. https://github.com/user-attachments/assets/fff1e842-0a6e-43cd-b833-3bc2f979fe8f --------- 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
Since there were a lot of conflicts as the older PR was before the slots, creating this off the latest main.
Added a
Dataprop underPrimary Header.withLogo Link.Screen.Recording.2025-12-11.at.10.57.37.PM.mov