-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Added Jsx Snippet Completion feature #45903
Added Jsx Snippet Completion feature #45903
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
2 similar comments
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple small things, and I’ll try it out in the meantime!
@typescript-bot pack this |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 318b59f. You can monitor the build here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol changes and the described behavior look good to me. Thanks for looking into this!
@typescript-bot pack this? |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 318b59f. You can monitor the build here. |
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot one thing!
I think it can be much smarter for boolean attributes. <input checke|| It should provide the following completions:
|
@Jack-Works We can only provide with one completion for each attribute, so in the case of "auto" we have decided that the best approach for booleans is to complete as little as possible. In you're example, that would be the third option: For users who always wants to have braces on their attributes, they will be able to configure the options with "braces" to achieve that. |
This isn’t true, but it is typical of current completions. I did think about an approach like @Jack-Works mentioned, but I personally didn’t find it compelling. It takes very few keystrokes to get from |
From From |
@Jack-Works We discussed offline a little bit about the snippet you propose and we decided to merge it as is to gather user feedback. I would suggest to submit this improvement as a suggestion to vscode and we can analyze further the best options. |
Implements suggestion #38891.