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

lucide-react - Generate types for all exported icons #243

Merged
merged 5 commits into from
Mar 17, 2021
Merged

lucide-react - Generate types for all exported icons #243

merged 5 commits into from
Mar 17, 2021

Conversation

FPDK
Copy link
Contributor

@FPDK FPDK commented Feb 27, 2021

Solves the React.js part of issue #226

The types in LucideProps are declared so they match the props mentioned in https://github.com/lucide-icons/lucide/blob/master/packages/lucide-react/README.md#props. They could be removed, since they're already present in SVGAttributes.

@ericfennis
Copy link
Member

@FPDK First of all, welcome to the Lucide Community!
Thanks for this PR, looking good. I will test this soon.

Yes partially, only strokeWidth could be removed. color and size are not SVGAttributes, so we need to keep them.

@ericfennis ericfennis added ⚛️ react package Lucide React Package 🐛 bug Something isn't working labels Feb 27, 2021
@lucide-icons lucide-icons deleted a comment from vercel bot Feb 27, 2021
@lucide-icons lucide-icons deleted a comment from vercel bot Feb 27, 2021
…e identical.

Fixed by adding an index attribute to the hash.
@vercel
Copy link

vercel bot commented Feb 27, 2021

@FPDK is attempting to deploy a commit to the Lucide Team on Vercel.

A member of the Team first needs to authorize it.

…butse are identical. Fixed by adding an index attribute to the hash."

This reverts commit 1c42b39
@FPDK
Copy link
Contributor Author

FPDK commented Feb 27, 2021

@ericfennis Thank you for the warm welcome 😄

Please ignore commit 1c42b39 in this pull request.
I have created a separate pull request for that issue: #244

Copy link
Member

@ericfennis ericfennis left a comment

Choose a reason for hiding this comment

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

@FPDK Nice work, thanks for your effort!
I came across some minor improvements we can make. I did also some learnings from other packages.

The types were working, I had no errors, but when hovering on the icon, the intellisense in my IDE was still saying: const Icon: any.

Also the IDE don't give suggestions:

So I try to figure out the problem, and I found out that the FC is breaking the typescript checking. This article makes it clear to me: typescript-react-why-i-dont-use-react-fc

So I made some code suggestions fixing this.

This is the result:

IDE also hinting props defined in the LucideProps interface

packages/lucide-react/build-types.js Outdated Show resolved Hide resolved
packages/lucide-react/build-types.js Outdated Show resolved Hide resolved
@FPDK
Copy link
Contributor Author

FPDK commented Mar 6, 2021

@ericfennis Thank you for the information and improvements. I'm gonna add React.FC to my naughty list :)

@vercel
Copy link

vercel bot commented Mar 7, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/lucide/lucide/9m7ysxVhHs2a24nmVrBCayX2vLqP
✅ Preview: Canceled

[Deployment for 6998ddb canceled]

@ericfennis
Copy link
Member

@FPDK Can you also move the build-types.js to the scripts folder?

packages/lucide-react/build-types.js ➡️ packages/lucide-react/scripts/build-types.js

@ericfennis
Copy link
Member

@FPDK I will merge this, and finish this. I want add to this in the next release.

Thanks for your contribution!

@ericfennis ericfennis merged commit 16a1ffc into lucide-icons:master Mar 17, 2021
@FPDK
Copy link
Contributor Author

FPDK commented Mar 18, 2021

@ericfennis sorry for not answering, glad you could finish it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working ⚛️ react package Lucide React Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants