Skip to content
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

DRY social icons #644

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

onnimonni
Copy link
Contributor

@onnimonni onnimonni commented Nov 1, 2024

@gauravmm did couple of nice refactorings in #629 like removing the repetition for the social icons. I took his changes and removed the DangerouslySetInnerHtml and now we have a nice way to configure the icons and colors from one single place.

If we want to merge this in smaller pieces merge #642 first.

I also added a short demo video to prove that they still work like they should:
Screen Recording 2024-11-01 at 14 47 55

Copy link

socket-security bot commented Nov 1, 2024

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@react-icons/[email protected]

View full report↗︎

Copy link

vercel bot commented Nov 1, 2024

@onnimonni is attempting to deploy a commit to the Saasify Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

charliechecks bot commented Mar 18, 2025

Pull Request Review: DRY Social Icons

Thank you for this PR that refactors the social icons implementation to reduce repetition. Based on the commit history, you've made several meaningful improvements:

Positive Aspects

  1. Reducing Code Duplication: Centralizing the definition of social icons and their colors makes the codebase more maintainable
  2. Better Package Management: Moving from @react-icons/all-files to react-icons seems like a good update
  3. Security Improvement: Removing dangerouslySetInnerHTML eliminates potential security vulnerabilities
  4. Code Cleanup: Removing unused parameters and improving formatting

Suggestions for Improvement

  1. Add Unit Tests: Consider adding tests to ensure the refactored components behave as expected
  2. Documentation: It might be helpful to add comments explaining the hover color implementation
  3. Consider TypeScript Typing: If not already implemented, adding proper type definitions would further improve maintainability

Questions

  1. What was the motivation behind replacing @react-icons/all-files with react-icons? Is it for better maintenance, smaller bundle size, or another reason?
  2. Could you share how the hover effect is now implemented without using dangerouslySetInnerHTML?

Overall, this refactoring appears to be a good improvement to the codebase - making it more maintainable while preserving functionality as shown in your demo.

@transitive-bullshit
Copy link
Owner

(testing out a new TS AI review bot)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants