-
Notifications
You must be signed in to change notification settings - Fork 0
chore: Centralise social platform metadata #118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Centralise social platform metadata #118
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR centralizes social platform configuration by extracting socialPlatforms data into a dedicated support module, updates component references to use the external source, standardizes SVG icon sizes from 16×16 to 24×24 viewBox in the header, and expands test coverage for header social links and icon rendering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Multiple interconnected changes across component refactoring, data structure introduction, UI updates, and test additions require reviewing logic flow across files, but changes follow consistent patterns with moderate logic density. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)tests/partials/HeaderPartial.test.ts (1)
tests/support/header-social-links.test.ts (1)
🔇 Additional comments (8)
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 |
Summary of ChangesHello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing code maintainability and consistency by centralizing the metadata for social media platforms. It extracts icon paths and descriptive text into a dedicated shared module, allowing various UI components to consume these standardized definitions. This approach reduces duplication and streamlines future updates to social platform information across the application. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request centralizes the social platform metadata, including icons and labels, into a shared support/social.ts module. This is a great improvement for maintainability, as it removes duplicated data from WidgetSocialPartial.vue and reuses the icons in HeaderPartial.vue. The changes are well-structured. I've found one potential issue related to case-sensitivity when looking up platform data, which could lead to social links not being displayed correctly. I've left a comment with details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/support/social.test.ts (4)
6-44: Consider structure-only validation to reduce brittleness.The tests correctly validate that each platform exports the expected icon and text properties. However, using
expect.stringContaining()with specific icon path fragments makes the tests brittle to icon updates.Consider validating just the structure (that icon and text exist and are non-empty strings) rather than checking specific icon content, unless the exact icon paths are critical to the API contract.
Example refactor:
- expect(socialPlatforms.x).toEqual( - expect.objectContaining({ - text: 'Latest Updates', - icon: expect.stringContaining('M13.3174 10.7749'), - }), - ); + expect(socialPlatforms.x).toEqual( + expect.objectContaining({ + text: expect.any(String), + icon: expect.any(String), + }), + ); + expect(socialPlatforms.x.text).toBeTruthy(); + expect(socialPlatforms.x.icon).toBeTruthy();
58-102: Clarify why only 2 of 3 platforms are included.The test provides 3 social entries (github, linkedin, instagram) but expects only 2 links (line 85). This filtering behavior depends on
headerSocialOrderorheaderSocialPlatformsfrom the implementation, but there's no explanation in the test.Add a comment explaining that the test validates the ordering and filtering behavior based on the header configuration, or explicitly verify the expected order.
Example comment:
it('builds ordered header social links with accessible copy', () => { + // Note: Only platforms defined in headerSocialOrder and headerSocialPlatforms + // will appear in results. Instagram is omitted because it's not in the header configuration. social.value = [
92-92: Extract hardcoded iconClass to reduce duplication.The iconClass value is duplicated across multiple assertions. Consider extracting it to a constant or referencing it from the source module to avoid brittleness if the class changes.
Example refactor:
+const expectedIconClass = 'fill-current text-slate-600 transition-colors hover:text-fuchsia-600 dark:text-teal-200 dark:hover:text-teal-300'; + expectLink(links[0], { name: 'github', url: 'https://github.com/example', label: 'Portfolio', title: 'Portfolio', icon: socialPlatforms.github.icon, - iconClass: 'fill-current text-slate-600 transition-colors hover:text-fuchsia-600 dark:text-teal-200 dark:hover:text-teal-300', + iconClass: expectedIconClass, });Also applies to: 100-100
104-142: Good coverage of fallback logic.The test correctly validates the fallback behavior when description is empty (falls back to handle) and when both are empty (falls back to platform name). The trimming behavior is also well-tested.
Optional: Consider adding an edge case for whitespace-only strings:
{ uuid: '7', name: 'youtube', url: 'https://youtube.com/example', description: ' ', // whitespace-only handle: ' ', // whitespace-only }This would verify that whitespace-only strings are treated the same as empty strings and fall back to the platform name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/support/social.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/support/social.test.ts (1)
src/support/social.ts (3)
socialPlatforms(10-31)HeaderSocialLink(38-45)useHeaderSocialLinks(60-91)
🔇 Additional comments (1)
tests/support/social.test.ts (1)
1-5: LGTM!The imports are correct and appropriately use the path aliases.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f08334c06c8333afe03eebffe3967e
Summary by CodeRabbit