Skip to content
This repository has been archived by the owner on Dec 5, 2024. It is now read-only.

fix placement type in PopperProps.children #148

Merged
merged 1 commit into from
Apr 27, 2018

Conversation

JLarky
Copy link
Contributor

@JLarky JLarky commented Apr 23, 2018

type is undefined | PopperJS.Placement, not ?PopperJS.Placement since that doesn't mean anything in typescript and was probably copied from Flow

fixes error that I get in typescript compiler:

node_modules/react-popper/typings/react-popper.d.ts
(24,16): JSDoc types can only be used inside documentation comments.

@FezVrasta
Copy link
Member

Thanks for the PR, we do have a test to verify that the ts types are right. May you check why isn't it detecting the problem please?

@JLarky
Copy link
Contributor Author

JLarky commented Apr 24, 2018

I was wondering about that myself. Maybe it has to do with typescript version. But since this was clearly the fix I wasn't going to investigate it too much.

@JLarky
Copy link
Contributor Author

JLarky commented Apr 24, 2018

@FezVrasta I decided to test it :)
image

so it has nothing to do with typescript version, just with config option that was suppressing this warning in your CI tests

@JLarky
Copy link
Contributor Author

JLarky commented Apr 24, 2018

And I guess just to be clear what this PR does:
image

@JLarky
Copy link
Contributor Author

JLarky commented Apr 26, 2018

@FezVrasta do you need anything else for this PR?

@giladgray
Copy link
Contributor

this PR fixes #152!
i also just opened #153 with a more weighty refactor to the types to make them more useful.

@JLarky
Copy link
Contributor Author

JLarky commented Apr 27, 2018

@giladgray I'm just waiting for any of them to be merged so I can start using popper inside my project :) but I guess one more PR is needed that would remove skipLibCheck that was underlying issue that made #152 possible

@FezVrasta
Copy link
Member

FezVrasta commented Apr 27, 2018

Thanks for looking into it, may you adjust the TS configuration so that it correctly detects this problem in the future? We can do everything in this PR

Also, fixed the TS config to detect this error in future.
@FezVrasta FezVrasta merged commit 73528bd into floating-ui:master Apr 27, 2018
@JLarky JLarky deleted the patch-1 branch April 27, 2018 22:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants