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

Create onLongPress.ts #19

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Create onLongPress.ts #19

wants to merge 20 commits into from

Conversation

vdegenne
Copy link

@vdegenne vdegenne commented Jun 6, 2023

No description provided.

Copy link
Owner

@43081j 43081j left a 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 Show resolved Hide resolved
src/directives/onLongPress.ts Outdated Show resolved Hide resolved
src/directives/onLongPress.ts Outdated Show resolved Hide resolved
src/directives/onLongPress.ts Outdated Show resolved Hide resolved
src/directives/onLongPress.ts Outdated Show resolved Hide resolved
src/directives/onLongPress.ts Outdated Show resolved Hide resolved
src/directives/onLongPress.ts Outdated Show resolved Hide resolved
// 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);
Copy link
Owner

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?

Copy link
Author

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.

Copy link
Author

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.

@43081j
Copy link
Owner

43081j commented Jul 11, 2023

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 bindInput tests work, but you can use test-element directly without needing a specific subclass i think.

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);
Copy link
Owner

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

@43081j
Copy link
Owner

43081j commented Jul 11, 2023

for the test cases, i can see roughly by eye we need these:

  • throws when non-element part is bound
  • calls callback when specified time passes during click
    • in this, assert that the pointer event is passed to the callback, and check that the currentTarget is the element we bound to
  • does not call callback when pointer leaves during specified time
  • removing the element and doing a long press doesn't call the callback
  • if no timer is specified, the default is used
  • reconnecting a removed element, then doing the long press calls the callback
  • if you change the callback between renders, then do a long press, the old one isn't called and the new one is
  • if you change the time between renders, then do a long press, the new time applies

you already have a couple of these. if you can fill the gaps in i think we're pretty much done

@mc2
Copy link

mc2 commented Feb 26, 2024

Docs please.

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.

None yet

3 participants