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.

Sign up for GitHub

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

Jump to bottom

Update event listener hooks #118

Merged
merged 7 commits into from
Apr 4, 2023
Merged

Update event listener hooks #118

merged 7 commits into from
Apr 4, 2023

Conversation

leroykorterink
Copy link
Collaborator

@leroykorterink leroykorterink commented Mar 31, 2023 •
edited
Loading

  • Remove useEventListener function overloads
  • Add RefObjectOption to useEventListener for ref support

#101 #102

@leroykorterink
Update event listener hooks
9c1957f 
- Remove useEventListener function overloads
- Remove useDocument hook
- Remove useWindow hook
- Add useDocument hook
- Add useWindow hook
- Add RefObjectOption to useEventListener for ref support

#101 #102
@leroykorterink leroykorterink added enhancement New feature or request good first issue Good for newcomers labels Mar 31, 2023
@leroykorterink leroykorterink requested a review from a team March 31, 2023 15:14
@leroykorterink leroykorterink self-assigned this Mar 31, 2023
leroykorterink
leroykorterink commented Mar 31, 2023
View reviewed changes
src/hooks/useEventListener/useEventListener.ts

export function useEventListener<T extends AbstractWorker, K extends keyof AbstractWorkerEventMap>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The overloads aren't correct and difficult to maintain, this may lead to bugs if types are not up-to-date. I think it's better to have correct types and write defensive code in the listener callbacks.

@leroykorterink leroykorterink mentioned this pull request Mar 31, 2023
Merged
ThaNarie
ThaNarie requested changes Mar 31, 2023
View reviewed changes
src/hooks/useDocument/useDocument.mdx Outdated Show resolved Hide resolved
src/hooks/useRefValue/useRefValue.ts Outdated Show resolved Hide resolved
src/hooks/useWindow/useWindow.mdx Outdated Show resolved Hide resolved
src/utils/getRefObjectOption/getRefObjectOption.mdx Outdated Show resolved Hide resolved
src/utils/getRefObjectOption/getRefObjectOption.test.ts Outdated Show resolved Hide resolved
leroykorterink added a commit that referenced this pull request Apr 3, 2023
@leroykorterink
Create isRefObject function
5f98187 
See: #118 (comment)
ThaNarie
ThaNarie requested changes Apr 3, 2023
View reviewed changes
src/utils/unref/unref.mdx Show resolved Hide resolved
@leroykorterink leroykorterink force-pushed the #102-allow-ref-object-in-use-event-listener branch from 5f98187 to 91b56ce Compare April 3, 2023 16:01
ThaNarie
ThaNarie reviewed Apr 4, 2023
View reviewed changes
src/utils/unref/unref.mdx Outdated Show resolved Hide resolved
@leroykorterink leroykorterink merged commit 09fe1e8 into main Apr 4, 2023
@leroykorterink leroykorterink deleted the #102-allow-ref-object-in-use-event-listener branch May 31, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmnathan mmnathan mmnathan left review comments

@ThaNarie ThaNarie ThaNarie approved these changes

Assignees

@leroykorterink leroykorterink

Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leroykorterink @ThaNarie @mmnathan