HarlemSquirrel / language-haml

Haml language grammar for GitHub's Atom IDE
MIT License
33 stars 24 forks source link

Fix link_to snippets #68

Closed darrenterhune closed 6 years ago

darrenterhune commented 7 years ago

I'd like to fix the link_to snippets. Currently they are as follows:

'link_to (route)':
  'prefix': '='
  'body': '= link_to "${1:Anchor Text}", ${2:route}_url'
'link_to (url)':
  'prefix': '='
  'body': '= link_to "${1:Anchor Text}", "${2:#}"'
'link_to (wrap selected text)':
  'prefix': '='
  'body': '= link_to "Anchor text...", ${1:"${2:$0}"}'

But as you can see the prefix is the exact same for all 3 options, so the last takes precedence I think and the others don't actually do anything.

IMO the options aren't well suited to, well suit everyones needs either.

1) link_to (route) not everyone would use the _url scheme. I personally never use that instead I use _path. I recommend we just change that to ${2:route} and let the user type it out. OR we add a 3rd ${2:route}_${3:path} 2)link_to (wrap selected text)would be nice if ${1:Anchor Text} was the first. 3) Because all of these current snippets are the same we'd need to define how they are triggered by differentprefixes` and be sure we are not over-riding something else with them.

Thoughts?

ezekg commented 7 years ago

Sorry I'm just now getting to this. I like your recommendation in point 1. I'll merge a PR if you're up for creating one with whatever you feel is best. I don't use snippets, so I'll leave it up to you.

HarlemSquirrel commented 7 years ago

Any thoughts, @darrenterhune ?

darrenterhune commented 7 years ago

Sorry guys, just been absolutely swamped. I think I did fix this, but didn't have enough time to review and create a pull request. Will try and get to this soon!

darrenterhune commented 7 years ago

I think we also need to over-ride this by adding another prefix https://github.com/atom/language-ruby-on-rails/blob/master/snippets/language-ruby-on-rails.cson#L541-L543

I'm not sure why they would have text.haml but I guess that's why you install other packages to provide the same functionality like this package 😄