Skip to content

Add new event modifier 'self' #3372

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

Closed
samuelgozi opened this issue Aug 7, 2019 · 9 comments
Closed

Add new event modifier 'self' #3372

samuelgozi opened this issue Aug 7, 2019 · 9 comments

Comments

@samuelgozi
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Event modifiers are long to type, and they could use shorter names or aliases, and in addition, it would be very helpful to have an additional self modifier that is very common.

Describe the solution you'd like
I would like to have the next aliases (inspiration from vue) for the modifiers:
stop as an alias for stopPropagation and prevent as an alias for preventDefault.

In addition, I would like to have a self modifier, that essentially checks that the clicked element is the one on which I added the event listener.

Describe alternatives you've considered
For the self modifier:

<script context="module">
        let el;
	function closeMenu(e) {
		if (e.target !== el) return;
                ...do stuff...
	}
</script>

<div bind:this={el} on:click|stopPropagation={doSomething}>
...
</div>

How important is this feature to you?
Events are at the very core of every reactive web app, I think that making it a little bit easier to use, is very important. In addition, It shouldn't be hard to do.

@antony
Copy link
Member

antony commented Aug 7, 2019

One thing I like about Svelte is that it's pretty close to vanilla Javascript. I disagree that shortening event modifiers is something desirable, as it's just another incline on the learning curve.

stop tells me nothing about what I'm stopping. stopPropogation tells me everything.

@pngwn
Copy link
Member

pngwn commented Aug 7, 2019

The shorthand syntax for event modifiers has been discussed before and decided against, we want to keep the event modifier names in line with the DOM APIs. Shorthand modifier names will not be implemented.

Regarding self, there is a simpler alternative to check the clicked element was the element that the listener was actually attached to that does not require an element binding:

<script context="module">
	function closeMenu(e) {
		if (e.target !== e.currentTarget) return;
                // do stuff...
	}
</script>

<div on:click|stopPropagation={doSomething}>
</div>

I can see the attraction of self, though I have few opinions on it myself, I'll see what others have to say on the matter.

@pngwn pngwn changed the title Shorten event modifiers names, and add a custom modifier Add new event modifier 'self' Aug 7, 2019
@pngwn pngwn added the proposal label Aug 7, 2019
@samuelgozi
Copy link
Contributor Author

samuelgozi commented Aug 7, 2019

I understand that you are trying to keep it close to vanilla js. But the proposal is not for replacing it, but merely to give the option of using shorthands.
And that doesn't add to the learning curve since when you go over that section of the docs you will clearly see that you can use the original naming, or the shorthand. And I'm willing to bet that the shortcut will be used more.

Adding a self modifier doesn't add complexity, the developer is expected to read about modifiers anyways, and he will probably see that there is a self modifier that "only trigger handler if event.target is the element itself".

In addition, the abstraction added above a self modifier is not greater than for the existing ones, and I'll explain why. When you add either of the modifiers preventDefault or stopPropagation behind the scenes Svelte is taking your function and decorating it with another function that calls these methods for you, so the original code is:

function calledOnClick(e) {
  ...do stuff...
}

But in runtime the same function will be called like:

function(event) {
	event.preventDefault();
	return calledOnClick.call(this, event);
};

Performing a self check will look something like this:

function(event) {
	if (event.target !== element) return;
	return calledOnClick.call(this, event);
};

Which is identical in the amount of work and abstraction that the compiled does for you.
And I believe that it doesn't add any conceptual overhead over the preventDefault or stopPropagation modifiers.

Edit: removed irrelevant point.

@pngwn
Copy link
Member

pngwn commented Aug 7, 2019

el was a typo in the original, I changed it shortly after, it was just meant to be e, the event object. As I said I have no opinion on this and am waiting for other responses.

Regarding additional syntax, we don't like adding additional ways of doing exactly the same thing like so it is still highly unlikely to be implemented. We prefer to optimise for readability rather than writability.

@samuelgozi
Copy link
Contributor Author

The readability part is understandable, about self I hope it's adopted.
I forked and implemented both, and will make a pull request on the self in a few minutes, and wait for a decision I guess.

@samuelgozi
Copy link
Contributor Author

My commit was merged.

@stalkerg
Copy link
Contributor

@samuelgozi pease update docs. Thanks for the feature.

@samuelgozi
Copy link
Contributor Author

@stalkerg my commit includes changes to the docs. Is there any specific thing I forgot?

@stalkerg
Copy link
Contributor

ah sorry, you right, I just checked 7342d48 commit and didn't find the full PR.
This issue has no link to PR :(

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

4 participants