Skip to content
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

Add a spec for "Snippets" #17329

Merged
merged 13 commits into from
Jun 22, 2024
Merged

Add a spec for "Snippets" #17329

merged 13 commits into from
Jun 22, 2024

Conversation

This comment has been minimized.

This comment has been minimized.

doc/specs/#1595 - Suggestions UI/Snippets.md Outdated Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Outdated Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Outdated Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Outdated Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Outdated Show resolved Hide resolved
Comment on lines +409 to +410
* [ ] The Command Palette and Suggestions UI need to be able to display both the
command name and a tooltip for the comment
Copy link
Member

Choose a reason for hiding this comment

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

For accessibility, remember to set the help text for the comment. Tooltips aren't really accessible since they may disappear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah but it isn't a Tooltip, it's something more ☺️

doc/specs/#1595 - Suggestions UI/Snippets.md Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 30, 2024
@zadjii-msft zadjii-msft added this to the Terminal v1.22 milestone May 31, 2024

This comment has been minimized.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 11, 2024
@zadjii-msft
Copy link
Member Author

zadjii-msft commented Jun 11, 2024

Notes from spec review:

  • We should have a toast to trust things in the CWD, ala vscode trusting workspaces (store in state)
  • Remove the whole section on wt.json in the command palette. f it. it can just be in the sxnui
  • Specific discussion on syntax for this file: JSON? fragment JSON? some other subset?
  • Do it like the snippets json below[1]
  • "The exact syntax as follows" is on wrong line
  • Just don't parse the saveSnippet action, cause what's even the point
  • clarify and elaborate on the "children supersede parents"
  • future consideration: snippets pane can prompt to add new snippet
  • leave wt save experimental for a while, probably until there's a dedicated save dialog. Commandline string escaping and quote parsing is neigh impossible
  • at least toast if we fail to load .wt.json for sxnui, snippets pane can also just display the error
  • v2: can we just use tasks.json syntax from vscode?
    • left as future consideration, cause those are FAR more powerful than what we're going to do now
  • do we need to do due diligence that we're reading arbitrary files off the file system?
  • Unanimous consent vote: .wt.json it is.

[1]: the json:

  {
    "$version": "1.0.0",
    "snippets":
    [
        {
            "input": "bx\r" 
            "name": "Build project",
            "description": "Build the project in the CWD"
        },

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 11, 2024
@zadjii-msft
Copy link
Member Author

b999c51 has all the feedback from the review yesterday

@zadjii-msft zadjii-msft removed the Needs-Attention The core contributors need to come back around and look at this ASAP. label Jun 12, 2024

This comment has been minimized.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Blocking over:

  • Command Palette discussion (like, adding in the discussion points)
  • discussion on .wt.json being next to the settings.json (see crazy idea below)

Crazy idea: what if we hard commit to snippets only being in .wt.json files?

Here's what I mean:

  • .wt.json files in the same directory as settings.json will be automatically loaded
  • (Migration) sendInput actions in settings.json will be moved to the local .wt.json file
  • Users can save .wt.json files from a separate repo to their local directory (besides the settings.json) so that they always have access to them
  • saveSnippet/x-save saves the snippet to the .wt.json

doc/specs/#1595 - Suggestions UI/Snippets.md Outdated Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Show resolved Hide resolved
doc/specs/#1595 - Suggestions UI/Snippets.md Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 12, 2024
@carlos-zamora carlos-zamora removed their assignment Jun 12, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 13, 2024
@zadjii-msft
Copy link
Member Author

Crazy idea: what if we hard commit to snippets only being in .wt.json files

Meh? I dunno, I'm not sure how valuable that is beyond just having a .wt.json that you stick in your dotfiles repo, then always stick in the root of the filesystem. Or just leaving then in the settings file. Just kinda feels like churn. I feel like all the user's settings they own should probably just stay in the one settings file we've already got.

I'd be open to promoting sendInput from just an action to just a general snippets property in the settings.json itself. That's not too crazy.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with me. Excited for Snippets! 🎉

@carlos-zamora carlos-zamora removed their assignment Jun 13, 2024
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

R+ let's GOOOO

doc/specs/#1595 - Suggestions UI/Snippets.md Outdated Show resolved Hide resolved
Copy link
Contributor

@PankajBhojwani PankajBhojwani left a comment

Choose a reason for hiding this comment

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

nervous excitement

@zadjii-msft zadjii-msft added this pull request to the merge queue Jun 22, 2024
Merged via the queue into main with commit 8511f3d Jun 22, 2024
28 checks passed
@zadjii-msft zadjii-msft deleted the dev/migrie/s/snippets branch June 22, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants