FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Added Jsx Snippet Completion feature by armanio123 · Pull Request #45903 · microsoft/TypeScript · GitHub

Added Jsx Snippet Completion feature - #45903

Merged
Armando Aguirre (armanio123) merged 5 commits into
microsoft:mainfrom
armanio123:AddJsxCompletionSnippet
Sep 22, 2021
Merged

Added Jsx Snippet Completion feature#45903
Armando Aguirre (armanio123) merged 5 commits into
microsoft:mainfrom
armanio123:AddJsxCompletionSnippet

Conversation

Copy link
Copy Markdown
Contributor

Implements suggestion #38891.

Copy link
Copy Markdown
Contributor

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 Sheetal Nandi (@sheetalkamat), Andrew Casey (@amcasey), Matt Bierner (@mjbvz), Mine Starks (@minestarks) for you. Feel free to loop in other consumers/maintainers if necessary

TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 16, 2021

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Copy link
Copy Markdown
Contributor

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good, just a couple small things, and I’ll try it out in the meantime!

Comment thread src/services/completions.ts Outdated
Comment thread src/services/completions.ts Outdated
Comment thread src/compiler/types.ts Outdated

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

TypeScript Bot (typescript-bot) commented Sep 16, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the tarball bundle task on this PR at 318b59f. You can monitor the build here.

Matt Bierner (mjbvz) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Protocol changes and the described behavior look good to me. Thanks for looking into this!

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this?

TypeScript Bot (typescript-bot) commented Sep 16, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the tarball bundle task on this PR at 318b59f. You can monitor the build here.

TypeScript Bot (typescript-bot) commented Sep 16, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Hey Andrew Branch (@andrewbranch), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/110739/artifacts?artifactName=tgz&fileId=EBEE052423D2275D919A988BA0A41126E538F286E254B7D4FC5DE13958A35E4202&fileName=/typescript-4.5.0-insiders.20210916.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.5.0-pr-45903-8".;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I forgot one thing!

Comment thread src/services/completions.ts Outdated

Copy link
Copy Markdown
Contributor

I think it can be much smarter for boolean attributes.

<input checke||

It should provide the following completions:

  • checked={|cursor_here|}
  • checked={false} |cursor_here|
  • checked

Copy link
Copy Markdown
Contributor Author

Jack Works (@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: checked. By the time this completions is provided, we don't fully know the intention of the user, so providing with a different option might be extra work for the user if it wants to have characters removed.

For users who always wants to have braces on their attributes, they will be able to configure the options with "braces" to achieve that.

Copy link
Copy Markdown
Member

We can only provide with one completion for each attribute

This isn’t true, but it is typical of current completions. I did think about an approach like Jack Works (@Jack-Works) mentioned, but I personally didn’t find it compelling. It takes very few keystrokes to get from checked to checked={|cursor here|}, and moving my fingers away from the home row to the arrow keys is more disruptive to my flow than typing those characters. More importantly, if VS Code ends up auto-selecting one of the longer options when you really want the short option, it’s super annoying. This is partly beyond our control since VS Code has their own settings and logic that can influence the pre-selected completion item. I do think it’s worth exploring these kinds of completions as we expand our snippet support, and I’m happy to hear other opinions, but I am initially biased against this one. Like Armando said, we’re starting out filling in the minimal high-confidence completion because typing code you do want is generally preferable to backspacing code you don’t want but got anyway, at least up to a point.

Jack Works (Jack-Works) commented Sep 21, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

From checked to checked={|cursor_here|} it need 3 key press: =, shift, { (and IDE will add }).

From checked={|cursor_here|} to checked only needs two: backspace (delete pair of {}), backspace (delete =).

Armando Aguirre (armanio123) merged commit 24e3b6b into microsoft:main Sep 22, 2021

Copy link
Copy Markdown
Contributor Author

Jack Works (@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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL