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

feat: add isSquare prop to enable square icons #402

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

Conversation

ali-ajam
Copy link

#401 This pull request introduces a new boolean prop, isSquare, to the SocialIcon component. With this prop, users can easily choose to render square icons instead of the default circular ones.

Screenshot 2025-02-18 at 9 54 18 PM Screenshot 2025-02-18 at 9 52 33 PM

@couetilc
Copy link
Owner

@ali-ajam thank you for your effort. I'd like you to make some changes.

  1. Rename the prop to square e.g. <SocialIcon square /> or <SocialIcon square={true} />.

  2. Remove the changes made to test/visual/spec.jsx. They are unnecessarily duplicative. That page is testing the icon definitions, not the border-radius css rule.

  3. In src/component.jsx, instead of using spread syntax to merge the key, simply set it conditionally in the body of the function e.g. if (square) social_svg.borderRadius = 0

Good job on the unit tests.

@ali-ajam
Copy link
Author

@ali-ajam thank you for your effort. I'd like you to make some changes.

  1. Rename the prop to square e.g. <SocialIcon square /> or <SocialIcon square={true} />.
  2. Remove the changes made to test/visual/spec.jsx. They are unnecessarily duplicative. That page is testing the icon definitions, not the border-radius css rule.
  3. In src/component.jsx, instead of using spread syntax to merge the key, simply set it conditionally in the body of the function e.g. if (square) social_svg.borderRadius = 0

Good job on the unit tests.

Thanks @couetilc, done.
Screenshot 2025-02-19 at 11 08 20 PM

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