feat: added link to the header logo #925
Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
WalkthroughAdds a new TranslatableString field Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (component render)
participant ExpandedHeader as ExpandedHeader Component
participant I18n as i18n Resolver
participant RouterLink as Link Component / Router
Browser->>ExpandedHeader: render with data.primaryHeader.logoLink
ExpandedHeader->>I18n: resolveTranslatableString(logoLink)
I18n-->>ExpandedHeader: resolvedLink (string)
ExpandedHeader->>ExpandedHeader: validate resolvedLink (LINK_REGEX_VALIDATION)
alt valid
ExpandedHeader->>RouterLink: render <Link href=resolvedLink>logo</Link>
RouterLink-->>Browser: clickable logo -> navigates
else invalid
ExpandedHeader->>RouterLink: render <Link href=DEFAULT_LOGO_LINK>logo</Link>
RouterLink-->>Browser: clickable logo -> DEFAULT_LOGO_LINK
end
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/visual-editor/src/components/header/ExpandedHeader.test.tsx (1)
444-517: Consider adding assertions for logo link functionality.While the snapshot tests verify visual rendering, the PR objectives mention several specific behaviors that should be tested:
- Link validation and fallback to DEFAULT_LOGO_LINK
eventName = "clickedLogo"aria-label = "Logo Link"The current tests only render the component without verifying these functional requirements. Consider adding assertions to verify:
- Valid URLs are rendered correctly as links
- Invalid URLs fall back to the default link
- The logo link has the correct aria-label
- Analytics events are fired with the correct event name
This would help prevent regressions if the validation logic, fallback behavior, or accessibility features are modified in the future.
packages/visual-editor/src/components/header/ExpandedHeader.tsx (3)
47-48: Consider strengthening URL validation.The regex pattern allows some edge cases that may not be valid URLs:
http://orhttps://without a domain would match- Special characters that are invalid in URLs are not filtered
However, if the intent is to provide flexible validation that also accepts relative paths and hash links, this might be acceptable.
Consider adding more robust validation:
-const LINK_REGEX_VALIDATION = /^(https?:\/\/[^\s]+|\/[^\s]*|#[^\s]*)$/; +const LINK_REGEX_VALIDATION = /^(https?:\/\/[a-zA-Z0-9\-._~:/?#[\]@!$&'()*+,;=]+|\/[^\s]*|#[^\s]*)$/;Or use a more comprehensive URL validation library for production use. You could also add a minimum length check for absolute URLs to prevent matching just the protocol.
419-424: Consider logging validation failures.The link resolution logic correctly validates and falls back to
DEFAULT_LOGO_LINK, but invalid links fail silently. This could make it difficult to debug configuration issues.Consider adding a console warning for development:
const inputLink = resolveComponentData(logoLink, i18n.language); const resolvedLink = typeof inputLink === "string" && LINK_REGEX_VALIDATION.test(inputLink.trim()) ? inputLink.trim() - : DEFAULT_LOGO_LINK; + : (console.warn(`[ExpandedHeader] Invalid logoLink "${inputLink}", falling back to ${DEFAULT_LOGO_LINK}`), + DEFAULT_LOGO_LINK);This would help content editors identify when their logo link configuration is invalid.
524-548: Improve aria-label for better accessibility.The
aria-label={t("logoLink", "Logo Link")}describes the technical nature of the element rather than its function. Screen reader users would benefit from a more descriptive label.Consider using a more user-friendly aria-label:
<Link href={resolvedLink} eventName="clickedLogo" - aria-label={t("logoLink", "Logo Link")} + aria-label={t("logoLinkAriaLabel", "Navigate to home")} >You would need to add the corresponding translation key
logoLinkAriaLabelto all locale files. This better communicates the action to users relying on assistive technologies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 10 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 10 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 11 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 15 props - fixed header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 15 props - sticky header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 24 props - logo Link - Invald.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 24 props - logo Link - Valid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] default props (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 10 props (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 10 props - secondary header (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 11 props - secondary header (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 15 props - fixed header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 15 props - sticky header (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 24 props - logo Link - Invald.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 24 props - logo Link - Valid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 10 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 11 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 15 props - fixed header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 15 props - sticky header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 24 props - logo Link - Invald.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 24 props - logo Link - Valid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (29)
packages/visual-editor/locales/cs/visual-editor.json(2 hunks)packages/visual-editor/locales/da/visual-editor.json(2 hunks)packages/visual-editor/locales/de/visual-editor.json(2 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(2 hunks)packages/visual-editor/locales/en/visual-editor.json(2 hunks)packages/visual-editor/locales/es/visual-editor.json(2 hunks)packages/visual-editor/locales/et/visual-editor.json(2 hunks)packages/visual-editor/locales/fi/visual-editor.json(2 hunks)packages/visual-editor/locales/fr/visual-editor.json(2 hunks)packages/visual-editor/locales/hr/visual-editor.json(2 hunks)packages/visual-editor/locales/hu/visual-editor.json(2 hunks)packages/visual-editor/locales/it/visual-editor.json(2 hunks)packages/visual-editor/locales/ja/visual-editor.json(2 hunks)packages/visual-editor/locales/lt/visual-editor.json(2 hunks)packages/visual-editor/locales/lv/visual-editor.json(2 hunks)packages/visual-editor/locales/nb/visual-editor.json(2 hunks)packages/visual-editor/locales/nl/visual-editor.json(2 hunks)packages/visual-editor/locales/pl/visual-editor.json(2 hunks)packages/visual-editor/locales/pt/visual-editor.json(2 hunks)packages/visual-editor/locales/ro/visual-editor.json(2 hunks)packages/visual-editor/locales/sk/visual-editor.json(2 hunks)packages/visual-editor/locales/sv/visual-editor.json(2 hunks)packages/visual-editor/locales/tr/visual-editor.json(2 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(2 hunks)packages/visual-editor/locales/zh/visual-editor.json(2 hunks)packages/visual-editor/src/components/header/ExpandedHeader.test.tsx(1 hunks)packages/visual-editor/src/components/header/ExpandedHeader.tsx(10 hunks)packages/visual-editor/src/docs/ai/components.d.ts(3 hunks)packages/visual-editor/src/docs/components.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/visual-editor/src/components/header/ExpandedHeader.tsx (4)
packages/visual-editor/src/types/types.ts (1)
TranslatableString(205-205)packages/visual-editor/src/editor/YextField.tsx (1)
YextField(188-318)packages/visual-editor/src/utils/index.ts (1)
resolveComponentData(19-19)packages/visual-editor/src/utils/resolveComponentData.tsx (1)
resolveComponentData(52-99)
packages/visual-editor/src/docs/ai/components.d.ts (1)
packages/visual-editor/src/types/types.ts (1)
TranslatableString(205-205)
⏰ 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 (22)
packages/visual-editor/locales/es/visual-editor.json (1)
227-227: ✓ Spanish translations added correctly.Both
logoLinkentries ("Enlace del logotipo") are properly placed within thefieldssection and at the top level. Translation is grammatically appropriate and consistent.Also applies to: 475-475
packages/visual-editor/locales/fr/visual-editor.json (1)
227-227: ✓ French translations added correctly.Both
logoLinkentries ("Lien vers le logo") follow the established pattern. Translation is natural and appropriate for French.Also applies to: 475-475
packages/visual-editor/locales/fi/visual-editor.json (1)
228-228: ✓ Finnish translations added correctly.Both
logoLinkentries ("Logo linkki") are properly positioned and consistent with the multilingual pattern.Also applies to: 476-476
packages/visual-editor/locales/hu/visual-editor.json (1)
228-228: ✓ Hungarian translations added correctly.Both
logoLinkentries ("Logo link") are consistently placed and properly formatted.Also applies to: 476-476
packages/visual-editor/locales/cs/visual-editor.json (1)
229-229: ✓ Czech translations added correctly.Both
logoLinkentries ("Odkaz na logo") are properly positioned with appropriate Czech phrasing.Also applies to: 477-477
packages/visual-editor/locales/pl/visual-editor.json (1)
229-229: ✓ Polish translations added correctly.Both
logoLinkentries ("Link do logo") are consistently placed and grammatically appropriate.Also applies to: 477-477
packages/visual-editor/locales/pt/visual-editor.json (1)
228-228: ✓ Portuguese translations added correctly.Both
logoLinkentries ("Link do logotipo") are properly positioned and linguistically appropriate.Also applies to: 476-476
packages/visual-editor/locales/de/visual-editor.json (1)
227-227: ✓ German translations added correctly.Both
logoLinkentries ("Logo-Link") are properly placed with appropriate German compound word formatting using hyphen.Also applies to: 475-475
packages/visual-editor/locales/et/visual-editor.json (1)
228-228: LGTM! Localization entry added correctly.The
logoLinktranslation key has been properly added to support the new logo link feature in the ExpandedHeader component.Also applies to: 476-476
packages/visual-editor/locales/zh/visual-editor.json (1)
228-228: LGTM! Chinese translation added correctly.The
logoLinktranslation follows the same pattern as other locale files in this PR.Also applies to: 476-476
packages/visual-editor/locales/en-GB/visual-editor.json (1)
227-227: LGTM! British English translation added correctly.The
logoLinktranslation has been added consistently with other locale files.Also applies to: 475-475
packages/visual-editor/locales/sk/visual-editor.json (1)
229-229: LGTM! Slovak translation added correctly.The
logoLinktranslation follows the established pattern across all locale files.Also applies to: 477-477
packages/visual-editor/locales/sv/visual-editor.json (1)
229-229: LGTM! Swedish translation added correctly.The
logoLinktranslation is consistent with the other locale additions in this PR.Also applies to: 477-477
packages/visual-editor/locales/it/visual-editor.json (1)
228-228: LGTM! Italian translation added correctly.The
logoLinktranslation maintains consistency with other locale files in this PR.Also applies to: 476-476
packages/visual-editor/src/docs/ai/components.d.ts (1)
410-410: LGTM! Logo link field added to header data interface.The
logoLinkfield has been correctly added toExpandedHeaderData.primaryHeaderas aTranslatableString, which aligns with the PR objectives to make the header logo clickable with multi-locale support.packages/visual-editor/locales/hr/visual-editor.json (1)
228-228: LGTM!The translation keys for
logoLinkare properly added in both locations (within thefieldssection and at the root level), which aligns with the translation structure used by other fields in the visual editor.Also applies to: 476-476
packages/visual-editor/locales/ro/visual-editor.json (1)
228-228: LGTM!The Romanian translations for
logoLinkare correctly added and follow the same pattern as other locale files.Also applies to: 476-476
packages/visual-editor/locales/en/visual-editor.json (1)
227-227: LGTM!The English translations for
logoLinkare properly added and will serve as the primary reference for other locales.Also applies to: 475-475
packages/visual-editor/src/components/header/ExpandedHeader.tsx (3)
78-78: LGTM!The
logoLinkfield is correctly typed asTranslatableString, which properly supports both constant values and localized content.
155-157: LGTM!The UI schema field definition for
logoLinkis properly configured as a translatable string field with appropriate internationalization support.
886-886: LGTM!The default value for
logoLinkis properly structured and consistent with other default link values in the component. While"#"is a safe placeholder, ensure users are guided to configure this to point to their actual homepage or desired destination.packages/visual-editor/src/docs/components.md (1)
248-251: LGTM!The documentation accurately reflects the addition of the
logoLinkfield to theExpandedHeadercomponent's data structure.
packages/visual-editor/src/components/header/ExpandedHeader.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/docs/components.md (1)
248-251: Consider enhancing the description for ExpandedHeader data props.The table correctly reflects the addition of
logoLink: TranslatableStringto thedata.primaryHeadertype. However, the description "Content for the main primary header bar" is generic and doesn't explain what individual fields do (e.g., thatlogoLinkmakes the logo clickable). Developers reading this documentation would benefit from either:
- A more detailed description explaining each field, or
- Individual table rows breaking down logo, logoLink, links, and CTAs separately.
That said, this matches the existing documentation style used for other complex types (e.g., ExpandedFooter). If you prefer consistency with the current approach, this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/docs/components.md(1 hunks)
⏰ 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
...ents/testing/screenshots/ExpandedHeader/[desktop] version 24 props - logo Link - Invaild.png
Outdated
Show resolved
Hide resolved
asanehisa
left a comment
asanehisa
left a comment
There was a problem hiding this comment.
lgtm besides screenshots merge conflict (sorry)
|
btw this has locale conflicts still |
|
@rkeerthient Can you fix the merge conflict? |
|
@benlife5, @asanehisa created a new PR here. Please disregard the current one. Thanks. |
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>
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>
3 participants
Ticket - https://yext.atlassian.net/browse/OPAQF-68?search_id=ef403121-0508-4063-915c-24698c7ce550.
DEFAULT_LOGO_LINK.clickedLogoand aria-label asLogo LinkScreen.Recording.2025-12-02.at.3.49.43.PM.mov