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

Use event listener composition #293

Merged
merged 7 commits into from
Aug 19, 2023

Conversation

stackoverfloweth
Copy link
Contributor

useEventListener

The useEventListener composition can be used to automatically handle setup and teardown of event listeners on the document or HTMElement scope. The options argument extends browser AddEventListenerOptions with immediate boolean, which defaults to true but when passed in as false, will prevent the composition from adding the listener automatically.

Example

import { useEventListener } from '@prefecthq/vue-compositions'

function handleEvent(event: MouseEvent) {
  // Respond to event
}
const element = ref<HTMLDivElement | undefined>()
useEventListener(element, 'keyup', handleEvent)

Arguments

Name Type Default
type K (K extends keyof DocumentEventMap) None
callback (this: Document, event: DocumentEventMap[K]) => unknown None
options AddEventListenerOptions & { immediate: boolean } None

Returns

Name Type Description
add () => void Manually attach the event listener (has no effect if the event listener already exists)
remove () => void Manually detach the event listener

@stackoverfloweth stackoverfloweth requested a review from a team as a code owner August 16, 2023 03:47
@stackoverfloweth
Copy link
Contributor Author

stackoverfloweth commented Aug 16, 2023

this PR also adds Vue 3.3 shims and updates useGlobalEventListener to simply wrap new useEventListener with document as target.

Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Couple questions but overall this is sweet!

src/useEventListener/README.md Outdated Show resolved Hide resolved
Comment on lines 32 to 34
if (getCurrentScope()) {
onScopeDispose(() => remove())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you use tryOnScopeDispose for this?

src/useEventListener/useEventListener.ts Outdated Show resolved Hide resolved
Comment on lines 40 to 43
watch(() => toValue(target), () => {
remove()
add()
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to be a little more sophisticated. If immediate is false I think this might add the listener anyway. Similar if remove is called but then the target changes this will automatically add the listener back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have a better, more elegant solution but it now seems to warrant some unit tests. Let me get some good tests written and I'll update this PR

Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Couple questions about the tests but overall looks really good to me 👍

])('given falsy target never adds event listener', (initialValue) => {
const callback = vi.fn()
const target = ref<HTMLElement | undefined | null>(initialValue)
vi.spyOn(utils, 'toValue').mockReturnValue(initialValue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? Won't toValue already return the initial value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow, this is so obvious in retrospect. Check out the updated tests, I think you'll find them significantly more concise

src/useEventListener/useEventListener.spec.ts Outdated Show resolved Hide resolved
src/useEventListener/useEventListener.spec.ts Outdated Show resolved Hide resolved
src/useEventListener/useEventListener.spec.ts Outdated Show resolved Hide resolved
src/useEventListener/useEventListener.spec.ts Show resolved Hide resolved
src/useEventListener/useEventListener.ts Show resolved Hide resolved
Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

❤️

@pleek91 pleek91 merged commit 34d8371 into PrefectHQ:main Aug 19, 2023
3 checks passed
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.

2 participants