-
Notifications
You must be signed in to change notification settings - Fork 30
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
Feature/success icons #391
Feature/success icons #391
Conversation
crypto-engineer
commented
Jul 17, 2022
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Running Lighthouse audit... |
1 similar comment
Running Lighthouse audit... |
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.
Hello @crypto-engineer, the code looks good to me 👍 However, I suggest we only extend the existing general Icon
component we already have in the component library for this use case. I noticed you also modified this general Icon
component so it contains the checkmark variant. Therefore - is there any reason for keeping the checkmark icon as a separate component please?
src/component-library/CheckCircleIcon/CheckCircleIcon.stories.tsx
Outdated
Show resolved
Hide resolved
Also, why is this PR to be merged into Interlay production please? |
@peterslany |
@peterslany |
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.
Thanks for the updates 👍