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

Update GHPRI link sharing contributions #4584

Merged
merged 17 commits into from
Mar 8, 2023

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented Mar 2, 2023

For microsoft/vscode#175676

This PR enables GHPRI copy link commands to show up in a new context menu that is anchored to editor line numbers. When a command contributed to the editor line number context menu is invoked, extensions will receive both the URI and line number that the copy link command was invoked for. I've modified the link generation code to observe that line number when it's available, instead of using the current selection as it does when invoked from the editor context menu or the command palette.

editorLineNumberContext.mp4

Additionally this PR

  • adds Copy github.dev Link when the user is in github.dev or a codespace
  • reworks Copy GitHub __ Link to generate github.com links to avoid redundancy
  • adds Copy GitHub __ Link actions to all share menus tracked by contribShareMenu proposed menus vscode#176316

@joyceerhl joyceerhl requested a review from alexr00 March 2, 2023 23:41
@joyceerhl joyceerhl changed the title Show Copy GitHub Link actions in proposed editor/lineNumber/context menu Update GHPRI link sharing contributions Mar 6, 2023
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, I just have a few questions!

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/issues/issueFeatureRegistrar.ts Show resolved Hide resolved
@joyceerhl joyceerhl requested a review from alexr00 March 7, 2023 22:49
@joyceerhl joyceerhl self-assigned this Mar 7, 2023
@joyceerhl joyceerhl added this to the March 2023 milestone Mar 7, 2023
Copy link
Member

@alexr00 alexr00 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 the PR! If the answer to #4584 (comment) is yes then feel free to merge!

{
"command": "issue.copyGithubDevLink",
"when": "github:hasGitHubRemotes && remoteName == codespaces || github:hasGitHubRemotes && embedderIdentifier == github.dev",
"group": "1_cutcopypaste@0"
Copy link
Member

Choose a reason for hiding this comment

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

nit: since this one is 1_cutcopypaste@0 I would put it first.

},
{
"command": "issue.copyGithubHeadLink",
"when": "github:hasGitHubRemotes && activeEditor == workbench.editors.files.textFileEditor && config.editor.lineNumbers == on",
Copy link
Member

Choose a reason for hiding this comment

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

Are all the config.editor.lineNumbers == on conditions needed? I would expect this to be controlled by VS Code core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it would make sense for core to handle hiding these commands, since the editor/lineNumber/context menu could also eventually include other extension-contributed items that we'd want to show regardless of line number enablement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This behavior also isn't final since I've already gotten feedback that it's unexpected that these commands would go away based on line numbers being toggled off to reduce visual noise.

@joyceerhl joyceerhl merged commit 8891228 into main Mar 8, 2023
@joyceerhl joyceerhl deleted the dev/joyceerhl/editorLineNumberContext branch March 8, 2023 15:37
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