-
Notifications
You must be signed in to change notification settings - Fork 3
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
Create onLongPress.ts #19
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry it took a while to get to this, i was away recently.
i've left a bunch of feedback. there's probably more once those changes are made, but its not far off
we also need some tests, but can do those after the other feedback
good idea for sure, thanks for the contribution 🥳
src/directives/onLongPress.ts
Outdated
// TODO: When the mouse is released and long press event | ||
// was accepted, we should find a way to cancel the @click | ||
// event listener if it exists. | ||
#onPointerDown = (e: Event): void => this.#initiateTimeout(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be a PointerEvent
too so we can drop the cast on line 98 below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use PointerEvent
there are incompatible type errors with the addEventListener
and removeEventListener
in above respective functions, not sure how to deal with those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with this solution, not sure if it's good though, I'll revert if you find it inappropriate.
last couple of comments are very minor but looks good other than those the last thing we need is a new test suite for the directive you can do similar to how the e.g. something along the lines of this: import '../util.js'; // so the test-element element is registered as a side effect
import {TestElement} from '../util.js';
suite('onLongPress directive', () => {
let element: TestElement;
setup(async () => {
element = document.createElement('test-element');
document.body.appendChild(element);
});
teardown(() => {
element.remove();
});
test('something', () => {
const spy = hanbi.spy();
element.template = () => html`
<div ${onLongPress(spy.handler)}></div>
`;
// simulate a long press by dispatchEvent() of individual pointer events in the right
// way to cause a long press
// assert that spy.called is true
});
}); |
suite('onLongPress directive', function () { | ||
let element: TestElement; | ||
|
||
this.timeout(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can probably drop this by just choosing smaller timers
you can go slightly less realistic with the values, you're still testing it correctly. e.g. the happy path test can just use 10ms or something, small enough that the default timeout of tests will be fine
for the test cases, i can see roughly by eye we need these:
you already have a couple of these. if you can fill the gaps in i think we're pretty much done |
Docs please. |
Closing as i finished it off in #31 |
No description provided.