Skip to content

Initial Notebook UI Mode in VS Code Insiders #2789

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

Merged
merged 36 commits into from
Jul 14, 2020

Conversation

TylerLeonhardt
Copy link
Member

PR Summary

This adds Notebook mode capabilities only in:

  • vscode insiders
  • when the user enables the setting: powershell.notebooks.showToggleButton
  • when registerNotebookContentProvider succeeds (we safely catch and log an error if it fails)

Added settings:

  • powershell.notebooks.showToggleButton - shows the button at the top right that will open your file in Notebook mode
  • powershell.notebooks.saveMarkdownCellsAs - new markdown cells need to either use block comments or line comments in the file. This setting determines which type of comment to use

Important notes:

  • This requires checking in a vscode.proposed.d.ts file that includes the typings for Notebook APIs
  • This also includes a GitHub Action that keeps that file up-to-date (it submits a PR) NOTE I NEED TO CHANGE THE BRANCH NAME WHEN WE'RE READY TO MERGE
  • Notebooks only work with ps1's. We could also support psm1s, but eh.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Comment on lines 126 to 127
"resolved": "https://botbuilder.myget.org/F/botframework-cli/npm/@types/node/-/@types/node-14.0.1.tgz",
"integrity": "sha1-XZPgoJnNCs1e89W948CG4fSf9ow=",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because we need a fast-release version for VSCode compat? Ideally we don't have to move off of npm (and to a repository with a deprecated signing hash algorithm) for long

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears that the vscode-dts is to blame here. When this API is stable, we wont need this so just ignore this for now.

kernel?: vscode.NotebookKernel;

constructor() {
this.kernel = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this property needed? It seems like we've declared it, so we should be able to pass this wherever we currently use this.kernel

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the vscode API uses it under the hood. I could potentially have 2 classes, one that implements vscode.NotebookContentProvider and one that implements vscode.NotebookKernel... but I felt one was fine, since vscode.NotebookKernel isn't that big of an interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha. This makes sense -- one of the great advantages of interfaces

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, this originally was one interface, but they broke it up into 2 recently.

kernel?: vscode.NotebookKernel;

constructor() {
this.kernel = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha. This makes sense -- one of the great advantages of interfaces

@TylerLeonhardt TylerLeonhardt requested a review from rjmholt July 10, 2020 23:04
@TylerLeonhardt
Copy link
Member Author

@rjmholt I've just added a couple tests :) I'll probably add more.

@TylerLeonhardt
Copy link
Member Author

rip codacy who knows where you went

@TylerLeonhardt TylerLeonhardt marked this pull request as ready for review July 13, 2020 20:35
@TylerLeonhardt
Copy link
Member Author

@rjmholt this should be ready now.

Co-authored-by: TylerLeonhardt <[email protected]>
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.

2 participants