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

Use label for "Follow link" command's tooltip #110917

Merged
merged 3 commits into from Nov 20, 2020

Conversation

dsanders11
Copy link
Contributor

Currently hovering over the "Follow link" command will show the URI for the command in text form. This PR changes it to showing the label as the tooltip.

Avoids this:

image

@gjsjohnmurray
Copy link
Contributor

Doesn't this change also mean I won't be able to preview the target of an http(s) link?

Maybe OK to omit the query string from what the hover currently shows.

Also, based on code reading I expected the link text in your screenshot to read "Execute command" rather than "Follow link".

@dsanders11
Copy link
Contributor Author

Doesn't this change also mean I won't be able to preview the target of an http(s) link?

True. Is that an intended use-case, or simply a side-effect of the current behavior? The PR could be adjusted so that it only uses the label if executeCmd is true.

Maybe OK to omit the query string from what the hover currently shows.

That would be an improvement, but I think showing the command name would still be undesirable behavior, as it's unexpected and not self-explanatory to anyone who doesn't know what command URIs are, as they're an implementation detail.

Also, based on code reading I expected the link text in your screenshot to read "Execute command" rather than "Follow link".

I believe that's because it's a Markdown document, and the markdown-language-features extension sets the tooltip to "Follow link", so that's what's displayed.

@gjsjohnmurray
Copy link
Contributor

🤷 Let's see what a team member says when this PR gets looked at.

@mjbvz mjbvz assigned alexdima and unassigned mjbvz Nov 19, 2020
@alexdima
Copy link
Member

I agree that the markdown tooltip is too much. Other test cases (http and file links):

<!--
    http://www.example.com
-->
<script src="script.js"></script>

@alexdima
Copy link
Member

Thank you!

@alexdima alexdima merged commit 87b8061 into microsoft:master Nov 20, 2020
@alexdima alexdima added this to the November 2020 milestone Nov 20, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 4, 2021
@dsanders11 dsanders11 deleted the patch-2 branch October 13, 2022 21:39
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.

None yet

4 participants