Skip to content

Improvement on snippets #9

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.

Sign up for GitHub

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

Jump to bottom
Closed
octref opened this issue Aug 25, 2017 · 2 comments
Closed

Improvement on snippets #9

octref opened this issue Aug 25, 2017 · 2 comments

Comments

@octref
Copy link
Contributor

octref commented Aug 25, 2017

Hey Sarah. Thanks for sharing your snippets by making it an extension and marking Vetur as a dependency.

In the demo, some stuff were not working correctly, so I'm here to ask for your setup and make the right PR to improve the snippets.

  1. Vetur offers some scaffold snippets, so vbase might be unnecessary

1

  1. I didn't see the snippets showing up in your gif (they should show up like IntelliSense items). For example, how did you get vanim to expand in the <template> in your gif?

Here is how it should be:

2

  1. Vetur divides vue files into "regions" with specific languages, and snippets should be declared according to the regions that they belong to. For example,
<template>
<!-- vue-html region -->
</template>
<!-- vue region -->
<style lang="scss">
.hi { // scss region
  color: red;  // scss region
}  // scss region
</style>
<!-- vue region -->

The idea is, each "region" has a language defined by lang value, with the exception of <template>, which has language vue-html.

Now you are marking everything as js snippets, so they would be available only in <script>, which is wrong for vanim etc:

3

The correct way is to divide template / js snippets into two files, and change the language associations (template to vue-html, js to javascript) in your contributes.snippets in package.json. Top level snippets, such as vbase should be associated for language vue.

Let me know how you want to proceed. I can soon send a PR that

  • removes vbase as it's covered by scaffold snippets
  • puts snippets into different files and associates them with correct languages
@sdras
Copy link
Owner

sdras commented Aug 28, 2017

Hi Pine!

Thanks for stopping by and reviewing the snippets, and thank you for Vetur. As you probably already guessed, my interest in Vetur as a dependency was not related to the snippets but to the additional support for Vue files, I definitely find it useful and the snippets couldn't work without that, which is an unfortunate side effect of how VS code languages has been set up, so thanks so much for enabling us the ability to use .vue in vscode.

  1. That said, I've gotten the most positive feedback for vbase and personally, I find it to be the most useful snippet. As mentioned in the readme, these snippets are less focused around API matching and more around real developer day to day ergonomics. I love using vbase and wouldn't accept a PR to remove it. I understand that you might have something similar in vetur, but in the event that I could one day remove the vetur dependency (this is my hope), I'd like them to stand on their own.

  2. The snippets weren't showing up in the intellisense in the original demo due to a syntax error that was corrected a few versions ago. Thanks for watching out, but it's resolved now.

  3. If you look back in the PRs, a few people needed the js version of the snippets as well because of the way they worked with JS before importing into Vue files. I allowed for this because, as mentioned before and stated in the readme, the purpose of these snippets are to be useful to common developers for everyday use.

Thanks for alerting me about vue-html, I'll likely make a refactor that reflects this, but it does work as it stands currently, in the vue json. Or, if you'd like to submit a PR for that (and not the vbase removal), I'd be happy to receive it. If you check out the readme, you can look over the script, html, and other snippets- the documentation separates these tables.

@sdras
Copy link
Owner

sdras commented Sep 9, 2017

This has been open for a few weeks now with no response, so I'm going to close it out. My last comment details what I think next steps could be, but I also don't think they're vital to the project. Thanks!

@sdras sdras closed this as completed Sep 9, 2017
sdras added a commit that referenced this issue Oct 14, 2017
@sdras
Merge pull request #10 from octref/master
70f6b17 
Snippet refactor for #9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sdras @octref