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

discuss clickOutside action #4

Open
swyxio opened this issue Nov 1, 2020 · 7 comments
Open

discuss clickOutside action #4

swyxio opened this issue Nov 1, 2020 · 7 comments

Comments

@swyxio
Copy link
Owner

swyxio commented Nov 1, 2020

considerations

  • this api puts the callback inside of the action parameters. other designs like this create events: <div use:clickOutside on:click_outside={handleClickOutside}>. is this better? seems verbose and indirect. perhaps a rule of thumb is if the action only creates one custom event, then just put it inside params
@TheComputerM
Copy link

A custom event is better imo

@dkzlv
Copy link
Contributor

dkzlv commented Nov 7, 2020

@sw-yx @TheComputerM

I would say callbacks are always better.
Firstly, it's more compact in the action itself: it's cb() against node.dispatchEvent(new CustomEvent('click_outside', node)).

Secondly, there can be some problems with Typescript. I'm not sure there is any solution to the typing issue, that this code

<div use:longpress on:longpress={console.log} />

now produces this error

Property 'onlongpress' does not exist on type 'HTMLProps<HTMLDivElement>'.

Same goes to all custom events. It's simple enough to handle in the VDom world, but I'm not sure how to handle it in Svelte except wrapping these DOM elements with components, which is mad.

@swyxio
Copy link
Owner Author

swyxio commented Nov 7, 2020

that's a great point. I wonder if there's any way to ignore that type error in svelte-2-tsx. but you're totally right

@russellsamora
Copy link

I think events seems like a more intuitive developer experience. Any new feelings on this or coming up with a standard? I love the idea of default actions. I see that longpress and clickOutside take the two different approaches at the moment though.

@swyxio
Copy link
Owner Author

swyxio commented Jan 20, 2021

yeah i'm aware - because contributed by different people haha.

there's some policymaking to be done here. eg nobody thinks we should have 3 callbacks for pannable. but we like callbacks for single events like clickOutside.

@SarcevicAntonio
Copy link

SarcevicAntonio commented Feb 23, 2021

Two things I would like to be considered when implementing this:

  1. when you have a menu for example you want the trigger to toggle the menu so you would have to exclude the trigger from the click outside somehow
    (Or am I missing something? Is this what the enabled Param and Update func is for?)

  2. when exporting as web component event.target will not be contained in node because of shadow dom, even when clicking inside
    We could use event.path / composedPath like this var path = event.path || (event.composedPath && event.composedPath()); and then use path[0] like event.target if (!node.contains(path[0])
    This works for shadow dom open

@schwartzmj
Copy link

  1. when you have a menu for example you want the trigger to toggle the menu so you would have to exclude the trigger from the click outside somehow

Just +1'ing this. We need a way to exclude elements or some other clever solution. e.g. clicking my mobile menu button ends up keeping the mobile menu open because I've both clicked outside and clicked the menu button.

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

No branches or pull requests

6 participants