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 custom JS events #122

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add custom JS events #122

wants to merge 5 commits into from

Conversation

lucasvallenet
Copy link
Contributor

Add /utils/events.js with addStartEvent and andEndEvents utils to easily bind an even on start/end with a debounce delay. Also added addScrollUpEvent and addScrollDownEvent to easily listen when the scroll direction updates

Usage of events :

addStartEvent

import { addStartEvent } from './utils/events'

// Detect document mousemove start with 1000ms delay
addStartEvent(document, 'mousemove', 1000)

document.addEventListener('mousemoveStart', () => {
   // ...callback
})

addEndEvent

import { addEndEvent } from './utils/events'

// Detect window resize end with 200ms delay (default value)
addEndEvent(window, 'resize')

window.addEventListener('resizeEnd', () => {
   // ...callback
})

addScrollUpEvent

import { addScrollUpEvent } from './utils/events'

// Add scroll up listener (binded by default to window)
addScrollUpEvent()

window.addEventListener('scrollUp', () => {
   // ...callback
})

addScrollDownEvent

import { addScrollDownEvent } from './utils/events'

// Add scroll down listener to specific DOM element
const $div = document.getElementById('#div')
addScrollUpEvent($div)

$div.addEventListener('scrollDown', () => {
   // ...callback
})

@mcaskill mcaskill changed the title JS events Add custom JS events Jun 1, 2022
Copy link
Member

@mcaskill mcaskill left a comment

Choose a reason for hiding this comment

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

Additional suggestions:

  • Should there be functions in place to disable custom events or remove base the event listeners that dispatch them?
  • I think the addScrollUpEvent and addScrollDownEvent could be refactored to use the same event listener on scroll.

Given the scope of this pull request, changes to /www/assets/styles should be excluded.

assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
assets/scripts/utils/events.js Outdated Show resolved Hide resolved
@lucasvallenet
Copy link
Contributor Author

I made the requested changed.

I also blended the addScrollUpEvent / addScrollDownEvent to a unique function. There is only one scroll listener and if we want to know when we scroll up, we usually also want to know when we scroll down...

Copy link
Member

@mcaskill mcaskill left a comment

Choose a reason for hiding this comment

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

All that is missing is to exclude /www/assets/styles.

@lucasvallenet
Copy link
Contributor Author

All that is missing is to exclude /www/assets/styles.

Didn't it reset on [0610a7d] ?

@mcaskill
Copy link
Member

mcaskill commented Jun 2, 2022

Didn't it reset on [0610a7d] ?

Oh, you reverted changes. I would suggest rebasing the master branch and dropping commits like that. Makes for a cleaner history.

@Jerek0 Jerek0 self-requested a review June 22, 2022 20:43
@Jerek0
Copy link
Member

Jerek0 commented Jun 22, 2022

👋 Thanks for the PR!

addStartEvent / addEndEvent

Some things worry me about this:

  • You can't remove a custom event as it stands. It's fine as long as you use it on document or window I guess, but if you setup a "start" event on a div that later disappears, the listener's still there and that's no good.
  • It can be used only once per EventTarget. I could imagine a situation where you want a "resizeend" on window with 200ms delay for one purpose, but somewhere else you need it to be 500ms delay.

Can you provide a concrete example where this would be useful aside from a global resizeend with a defined debounce delay? I wonder if that sort of custom event with a debounce should be done manually where needed only?

addScrollDirectionEvents

Regarding the scroll up/down events, it works well but I wonder if it's the right place for such a feature. Maybe it should be inside the Scroll module of the project instead? Also, we may want to prevent calling addScrollDirectionEvents multiple times on the same EventTarget.


By the way, it didn't work right away during my tests. I figured some variables/functions calls didn't exist, it's now fixed ✅

@arnvvd
Copy link
Contributor

arnvvd commented Oct 5, 2022

Outcome from meeting on 2022-10-05
Status: On hold

  • Something to bring and discuss with Locodash

@arnvvd arnvvd added the on hold label Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants