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

[ShadCN] refactor(icons): use react-icons IconBase composition #13970

Merged
merged 14 commits into from
Oct 15, 2024

Conversation

TylerAPfledderer
Copy link
Contributor

Description

Related Issue

Copy link

netlify bot commented Sep 26, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit ce1f2e9
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/670e7859dc003400081f37e5
😎 Deploy Preview https://deploy-preview-13970--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 44 (🔴 down 9 from production)
Accessibility: 93 (no change from production)
Best Practices: 89 (🔴 down 9 from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

@TylerAPfledderer this looks great! didn't know we had a IconBase in react-icons.

cc @wackerow

defihook

This comment was marked as spam.

@pettinarip
Copy link
Member

Hey @TylerAPfledderer would you need some help on this? its gaining priority now that we need to migrate a bunch of components that use these icons. Would be nice to have this in place to avoid converting all the icons to svgs.

I'll test it locally in the meantime.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip yes, that would be helpful. Getting bogged down over here at the moment.

I would point out that when double-checking changes to the icons where rendered in components/pages, that if a color was added, it might need to be added to the fill css property instead of text color.

text-* => fill-*

@pettinarip
Copy link
Member

Ok @TylerAPfledderer.

I've reverted the StakingHierarchy related icons to migrate them later as they are a bit related to that component logic. I don't think we need to address that in this PR.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

@TylerAPfledderer I have tested this and it looks good to me. I wonder if you are working on or expecting to do more things in this PR.

Left a comment but I think we should tackle that in a separate PR.

@@ -1,7 +1,14 @@
import type { SVGAttributes } from "react"
import { IconProps } from "@chakra-ui/react"

export const commonIconDefaultProps: IconProps = {
Copy link
Member

Choose a reason for hiding this comment

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

This is the next iteration we need to do, right? replace this with commonIconDefaultAttrs. If you agree, we can tackle that in a different PR, just to avoid blocking this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip Yes, that is correct. Thank you!

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip I have not considered anything additional for this PR.

@github-actions github-actions bot added the tooling 🔧 Changes related to tooling of the project label Oct 8, 2024
@pettinarip pettinarip marked this pull request as ready for review October 15, 2024 13:47
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

@TylerAPfledderer I fixed a few minor things. Overall I think the icons work well. Thanks for the hard work on the refactor.

@pettinarip pettinarip merged commit bf3f122 into ethereum:dev Oct 15, 2024
10 checks passed
@TylerAPfledderer TylerAPfledderer deleted the refactor/icons-to-IconBase branch October 15, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants