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

Hover & Focus state for Social Icons #670

Merged
merged 2 commits into from Oct 11, 2023

Conversation

adetyaz
Copy link
Contributor

@adetyaz adetyaz commented Oct 5, 2023

Please describe the changes this PR makes and why it should be merged:
This solve the issue #649

Status

  • Code changes have been tested against prettier, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the codebase
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
    • This PR changes the internal workings with no modifications to the external API (bug fixes, performance improvements)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@vercel
Copy link

vercel bot commented Oct 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
million-kitchen-sink ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 11, 2023 8:56pm
sink ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 11, 2023 8:56pm

@CLAassistant
Copy link

CLAassistant commented Oct 5, 2023

CLA assistant check
All committers have signed the CLA.

@adetyaz
Copy link
Contributor Author

adetyaz commented Oct 5, 2023

Hey @tobySolutions,
I had an issue when trying to make sure only the changes I made were committed so I just did it all over again when I couldn't solve the issue

Copy link
Contributor

@tobySolutions tobySolutions left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tobySolutions
Copy link
Contributor

What's the. advantage of the raw svg code over using React icons, please? @adetyaz

Copy link
Contributor

@tobySolutions tobySolutions left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can make the svgs an exportable component that you can come use here instead of having all the svg code be here; thank you.

@adetyaz
Copy link
Contributor Author

adetyaz commented Oct 7, 2023

Noted @tobySolutions I will do that and update.

I used SVG because that was that's what I saw when digging through the Nextra documentation

@tobySolutions
Copy link
Contributor

Noted @tobySolutions I will do that and update.

I used SVG because that was that's what I saw when digging through the Nextra documentation

Awesome! Thanks.

@tobySolutions
Copy link
Contributor

Oh, you haven't made the updates yet?

@adetyaz
Copy link
Contributor Author

adetyaz commented Oct 8, 2023

Oh, you haven't made the updates yet?

Not yet, I'll be doing that soon. I'd like to know if I should create an icons folder in the components folder and put each one there since the have different svg values that can't be changed via props

@tobySolutions
Copy link
Contributor

Any updates on this @adetyaz?

@adetyaz
Copy link
Contributor Author

adetyaz commented Oct 11, 2023

Yes I'll be pushing my updates in about an hour, I was waiting for a response on the question I asked

@adetyaz
Copy link
Contributor Author

adetyaz commented Oct 11, 2023

@tobySolutions It has been updated

@tobySolutions
Copy link
Contributor

Can you please sign the CLA? @adetyaz

@adetyaz
Copy link
Contributor Author

adetyaz commented Oct 11, 2023

Can you please point me to it @tobySolutions

@tobySolutions
Copy link
Contributor

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

@adetyaz

@adetyaz
Copy link
Contributor Author

adetyaz commented Oct 11, 2023

done @tobySolutions

@tobySolutions
Copy link
Contributor

done @tobySolutions

Awesome! Good job @adetyaz

Copy link
Contributor

@tobySolutions tobySolutions left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

@tobySolutions tobySolutions merged commit 0ddf394 into aidenybai:main Oct 11, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants