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

Add missing DropTargetMoniter type arguments #3261

Closed
wants to merge 1 commit into from
Closed

Add missing DropTargetMoniter type arguments #3261

wants to merge 1 commit into from

Conversation

equt
Copy link

@equt equt commented Jul 17, 2021

This should allow cases like

to have type inference.

@equt equt requested a review from darthtrevino as a code owner July 17, 2021 01:30
@equt
Copy link
Author

equt commented Aug 6, 2021

Wait, is this a bug of GitHub?

@darthtrevino
Copy link
Member

Oh that's weird, it nuked your branch? I merged some other PRs that were pending. It looks like this is a bot you installed on your fork?

@equt
Copy link
Author

equt commented Aug 6, 2021

Yes, I installed the pull bot for synchronizing my forks, it basically does a simple fetch and force push I guess. Well, it's my bad to use it globally.

But the problem here is why the CI is triggered? This is my first pull request in this repository, so it should be blocked. Did you trigger that @darthtrevino?


I've recovered it, and it still gets blocked. So either this bot has contributed to this repo, or there is a potential bug to bypass the maintainer's approval.

@darthtrevino
Copy link
Member

Hey @equt - I'm considering this one. It definitely improves the typings, but it compromises the development ergonomics. A developer would now have to specify 3 generic parameters explicitly when using useDrag(). I've tried to make that implicit as possible, which is why I've left those generic args out, but I'm open to reconsidering.

@equt
Copy link
Author

equt commented Aug 13, 2021

The actual reason for this pull request is that I found the generic arguments for the monitor of DragSourceHookSpec is provided

collect?: (
monitor: DragSourceMonitor<DragObject, DropResult>,
) => CollectedProps

However the one for the DropTargetHookSpec is not

collect?: (monitor: DropTargetMonitor) => CollectedProps

@mifozski
Copy link

mifozski commented Jan 25, 2022

Why not also provide the arguments to the rest of the DropTargetHookSpec's monitor usages? Like in drop, hover signatures, etc.

@darthtrevino
Copy link
Member

These types have been incorporated in #3300; thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants