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

Fix Typescript definitions to allow for props forwarded to SVG element: style, onClick, etc. #50

Open
wants to merge 1 commit into
base: master
from

Conversation

@pcorpet
Copy link

commented Apr 13, 2019

Fixes #48.


This change is Reviewable

@levrik

This comment has been minimized.

Copy link
Owner

commented Apr 16, 2019

Awesome. Thanks!
As soon as I have time I will take a look at it. Probably next week.

@pcorpet
Copy link
Author

left a comment

Up. Can you take a look please?

Reviewable status: 0 of 4 files reviewed, all discussions resolved

@levrik

This comment has been minimized.

Copy link
Owner

commented Jul 14, 2019

@pcorpet I don't quite understand the combination of Pick and Exclude. Could you elaborate on that?

@pcorpet

This comment has been minimized.

Copy link
Author

commented Jul 14, 2019

Pick + Exclude is the equivalent of Omit before Omit was invented:

Pick<SVGProps, Exclude<keyof SVGProps, ReservedProps>>

is the equivalent (inTypescript 3.5) of

Omit<SVGProps, ReservedProps>

See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-5.html#the-omit-helper-type

Which is taking the SVGProps interface but removing the fields mentionned in ReservedProps (size, width, height, fill, viewBox). I'm omitting width & height as you should not set them. I'm omitting size because we're changing its type, and finally omitting fill & viewBox because if you set them on the icon externally those properties are overriden by the implementation.

The exact detail is: Pick<A, Exclude<B, C>>
Pick the fields of A that are in B but not in C, which in my case: pick the fields of SVGProps that are in (keyof SVGProps) but are not in ReservedProps.

@pcorpet

This comment has been minimized.

Copy link
Author

commented Aug 6, 2019

@levrik A friendly ping.

@levrik

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2019

@pcorpet Hey. Sorry. Got it now. I'm gonna merge and release it the coming weekend.

@levrik

levrik approved these changes Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.