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

Finalize NotebookEditor api proposal #149767

Merged
merged 5 commits into from
May 23, 2022
Merged

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented May 17, 2022

Fixes #149271

This finalizes most parts of the NotebookEditor api proposal. I haven't removed the proposal entirely as there are still a few parts being left behind:

  • The deprecated properties/functions
  • A few contribution points such as notebook/cell/executePrimary

Fixes microsoft#149271

This finalizes most parts of the NotebookEditor api proposal. I haven't removed the proposal entirely as there are still a few parts being left behind:

- The deprecated properties/functions
- A few contribution points such as `notebook/cell/executePrimary`
@mjbvz mjbvz added this to the May 2022 milestone May 17, 2022
@mjbvz mjbvz requested review from rebornix and jrieken May 17, 2022 21:11
@mjbvz mjbvz self-assigned this May 17, 2022
@mjbvz
Copy link
Collaborator Author

mjbvz commented May 17, 2022

@rebornix and @jrieken I just realized that the notebookEditor proposal is also used to gate the following menu contributions:

  • notebook/cell/executePrimary
  • interactive/toolbar
  • interactive/cell/title

Let me know if these can also be finalized or should be split into a new proposal

@mjbvz mjbvz enabled auto-merge (squash) May 18, 2022 03:35
@jrieken
Copy link
Member

jrieken commented May 18, 2022

notebook/cell/executePrimary sounds a bit exotic. May this should be notebook/cell/execute with a special group (of length 1) which is rendered prominent. /cc @roblourens for background

@jrieken
Copy link
Member

jrieken commented May 18, 2022

Also debug/toolBar vs notebook/toolbar vs interactive/toolbar (only the latter isn't finalized)

@roblourens
Copy link
Member

That executePrimary menu is only needed for remotehub's "Continue Working On" prompt and it will go away when that moves to core. #149271 (comment) I think that menu shouldn't be finalized. Sorry for not calling that out.

@mjbvz
Copy link
Collaborator Author

mjbvz commented May 18, 2022

I've confirmed that interactive/cell/title and interactive/toolbar are being used by the Jupyter extension: https://github.com/microsoft/vscode-jupyter/blob/584e7e126dc1d218909e8277a6e2bb3d65315892/package.json#L1009

Not sure if there are any other users. @roblourens Do you think we can safely finalize these two contributions? Or do they need to remain proposed

@rebornix
Copy link
Member

@mjbvz I think both interactive/cell/title and interactive/toolbar are stable and good for finalization. They are similar to notebook/cell/title and notebook/toolbar and they are used uniquely in Interactive Window.

@mjbvz mjbvz disabled auto-merge May 23, 2022 21:55
@mjbvz mjbvz enabled auto-merge (squash) May 23, 2022 21:55
@mjbvz mjbvz disabled auto-merge May 23, 2022 23:06
@mjbvz mjbvz enabled auto-merge (squash) May 23, 2022 23:06
@mjbvz mjbvz merged commit 45304da into microsoft:main May 23, 2022
mjbvz added a commit to mjbvz/vscode that referenced this pull request May 23, 2022
Follow up on microsoft#149271

Finalizing these two contributions based on microsoft#149767 (comment)

We're leaving the `notebook/cell/executePrimary` point for now as we don't plan to finalize it
mjbvz added a commit that referenced this pull request May 23, 2022
Follow up on #149271

Finalizing these two contributions based on #149767 (comment)

We're leaving the `notebook/cell/executePrimary` point for now as we don't plan to finalize it
@github-actions github-actions bot locked and limited conversation to collaborators Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finalize notebookEditor API proposal
4 participants