corneliusio / svelte-sublime

💡Sublime Text syntax highlighting for Svelte components
MIT License
59 stars 6 forks source link

Scope for event handlers should be changed #6

Closed ccampbell closed 4 years ago

ccampbell commented 5 years ago

This is more of an opinion thing, but I noticed this:

image

Regular attributes use the scope entity.other.attribute-name.html, the svelte keywords use support.function.svelte which I think is reasonable, but the name of the event uses string.unquoted.svelte which I think is a bit strange.

I actually think the more correct way would be to wrap the entire out:fade with the scope entity.other.attribute-name.html.svelte and then entity.other.function.svelte for out and entity.other.property.svelte for fade. This means it would preserve the normal attribute syntax highlighting from HTML, but you have the option to customize it in your theme for Svelte specifically which in general is the Sublime way of doing things. In HTML, that would look like this

<span class="entity other attribute-name html svelte">
    <span class="entity other function svelte">out</span><span class="punctuation separator svelte">:</span><span class="entity other property svelte">fade</span>
</span>

It is just an idea. Feel free to close if you disagree :smile:

corneliusio commented 4 years ago

Hey, I'm sorry for never getting to this!

This is partially a hold over from the the fact that I started writing these syntax definitions back before svelte added animations so all foo:bar attributes were only event listeners where it makes sense that it's equivalent to something like el.on('event', onEvent). Then as new features were added I simply used the same matching for everything since that was simpler and I actually liked the consistency of it.

That said, it does make sense to switch these up and probably some others too. I'll start taking a look at this (quarantine's got me catching up on all sorts of things. 😅).

corneliusio commented 4 years ago

Ok, so think this is where I've landed.

Svelte attributes will now be entirely wrapped with the scope entity.other.attribute-name.svelte. I didn't go with with entity.other.attribute-name.html.svelte as this does not appear to be a convention followed by other frameworks such as Vue or React.

(on|bind|class|use|in|out|transition|animate) are all essentially support functions provided by Svelte, so I think it makes the most sense to leave them as support.function.svelte.

(in|out|transition|animate|use) are basically functions that accept a function as an argument. So something like transition:fly translates to transition(fly) where fly is an easing function imported into the component scope. So I've updated these "arguments" to be variable.other.readwrite.svelte (Didn't go with something like variable.function.svelte since they are not being invoked in the current context, similar to onClick below).

(on|bind|class) are all functions accepting a string argument. For example, on:click={onClick} is essentially el.on('click', onClick) where click is not a variable or function found in the component scope the way the transition easing functions are. So I've left these string.unquoted.

let is the one exception to the above as it is functioning as a storage type, so I've given it the scope of storage.type.svelte and it's "argument" a scope of variable.other.readwrite.svelte

Also, as a bonus, I've given modifiers their own scope definitions as well so they are not simply bundled up with whatever precedes them.

Hope this all makes sense, it should offer a bit more control for syntax highlighting and be considerably more accurate then the previous definitions. I know it's been a while since you submitted this, but if you have any feedback or concerns feel free to post them. Otherwise I will likely publish the changes in the next day or so.

corneliusio commented 4 years ago

I've gone ahead and published these changes in 3.2.0.