Conversation
Move closest() and once() tests into separate files Add additional once() tests
Antonio-Laguna
left a comment
Antonio-Laguna
left a comment
There was a problem hiding this comment.
Great stuff @mturnwall !
I left a couple of comments on some places. Please let me know of your thoughts.
README.md
Outdated
| #### Parameters: | ||
|
|
||
| `element` - {Element} the element where you want to start looking from | ||
| `origin` - {string:Element} - the element where you want to start looking from. It can be either a valid CSS selector or an Element. |
There was a problem hiding this comment.
Nitpicky. Looking like JSDoc here. Can we have string|Element (union type). Seems correct down below for once
There was a problem hiding this comment.
Just a typo, it should have been |
lib/once.js
Outdated
| off(el, event); | ||
| function once(selector, event, run, capture = false) { | ||
| on(selector, event, e => { | ||
| off(selector, event); |
There was a problem hiding this comment.
Unrelated to this PR but as per docs, the capture should be passed on to off too.
There was a problem hiding this comment.
off() doesn't receive a capture option since removeEventListener() doesn't have one. The docs have this off(selector, event)
There was a problem hiding this comment.
It does, according to MDN. Is there something I'm missing?
test/closest.test.js
Outdated
| import domassist from '../domassist'; | ||
| import test from 'tape-rollup'; | ||
|
|
||
| function addNode(el, num) { |
There was a problem hiding this comment.
Although this test is similar than the previous one, I think we should just have a template with a clear structure on how it looks and what's the expected result. Look at what we do with Domodules. What do you think about it?
There was a problem hiding this comment.
Can you point me to what you are referring to? I'm not sure I understand.
There was a problem hiding this comment.
|
LGTM |
3 participants
Move closest() and once() tests into separate files
Add additional once() tests