swyxio / svelte-actions

prototype official actions for Svelte
MIT License
228 stars 9 forks source link

discuss `clickOutside` action #4

Open swyxio opened 3 years ago

swyxio commented 3 years ago

considerations

TheComputerM commented 3 years ago

A custom event is better imo

dkzlv commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago
  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.