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

Test and iterate on TS expandable hover apis #232685

Open
mjbvz opened this issue Oct 31, 2024 · 7 comments
Open

Test and iterate on TS expandable hover apis #232685

mjbvz opened this issue Oct 31, 2024 · 7 comments
Assignees
Labels
editor-hover Editor mouse hover feature-request Request for new features or functionality on-testplan
Milestone

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 31, 2024

To try it:

Let's test out TS's current implementation of expandable hovers. The main question is if the UI is good enough as-is, or if it turns out we need something different, which may require API changes

@mjbvz mjbvz added this to the November 2024 milestone Oct 31, 2024
@aiday-mar aiday-mar added the editor-hover Editor mouse hover label Oct 31, 2024
@aiday-mar
Copy link
Contributor

aiday-mar commented Oct 31, 2024

Tagging @gabritto about the following

I followed the steps in order to use the TypeScript compiler from the branch. Some feedback I have:

In the vscode repo on line 202 of the file contentHoverController.ts, hover on this._contentWidget, this shows an expandable hover. Expand it, this creates a big hover with the expanded type of the content widget. Aside from the fact that the hover is really long, which I am not sure can be avoided, I noticed several things:

  • The hover content is cut off, with an ellipsis at the end. Either the editor hover cuts off content or TypeScript truncated the editor text. In this case I would have liked to see the full type.
  • Curiously there is incorrect tokenization/colorization of the code in the hover. I am not sure if this is somehow related to the TreeSitter work that was recently done or if this has always been like this.
  • As we previously discussed, it would be good to have clickable links that expand the hover instead of expanding everything at the same time. Expanding everything makes the hover unnecessarily long and hard to navigate.
Screen.Recording.2024-10-31.at.15.47.58.mov

I think it would be good generally to change the API so that we allow linkification of markdown text for the purpose of expansion. Perhaps in the coming iterations, I can develop this, if there is sufficient need for this to be put into the iteration plan.

cc @alexdima

@gabritto
Copy link
Member

Tagging @gabritto about the following

I followed the steps in order to use the TypeScript compiler from the branch. Some feedback I have:

In the vscode repo on line 202 of the file contentHoverController.ts, hover on this._contentWidget, this shows an expandable hover. Expand it, this creates a big hover with the expanded type of the content widget. Aside from the fact that the hover is really long, which I am not sure can be avoided, I noticed several things:

  • The hover content is cut off, with ellipsis at the end. Either the editor hover cuts off content or TypeScript truncated the editor text. In this case I would have liked to see the full type.
  • Curiously there is incorrect tokenization/colorization of the code in the hover. I am not sure if this is somehow related to the TreeSitter work that was recently done or if this has always been like this.
  • As we previously discussed, it would be good to have clickable links that expand the hover instead of expanding everything at the same time. Expanding everything makes the hover unnecessarily long and hard to navigate.

Screen.Recording.2024-10-31.at.15.47.58.mov
I think it would be good generally to change the API so that we allow linkification of markdown text for the purpose of expansion. Perhaps in the coming iterations, I can develop this, if there is sufficient need for this to be put into the iteration plan.

cc @alexdima

Thanks for the feedback. I removed truncation of the hover content on the TSServer side when expandable hovers are requested, so I think that's happening on the vscode side.

@aiday-mar
Copy link
Contributor

aiday-mar commented Nov 1, 2024

I see thanks for letting me know, I'll have a look to remove truncation of hovers. I will discuss this issue with my manager to see if we should place the linkification of markdown text into the iteration plan and work on it.

I suppose also we could do a test pass on this perhaps during release week to get feedback about this.

@aiday-mar
Copy link
Contributor

Hi I discussed this with my manager. Could we merge and publish the TypeScript branch work to production so we can self-host easily on the work in VS Code?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 6, 2024

Hey @aiday-mar, the PR was merged yesterday afternoon. You all should be able to pick it up in the current nightly (5.8.0-dev.20241106), but let me know if there are any issues with that.

@aiday-mar
Copy link
Contributor

Hi @DanielRosenwasser thanks for letting me know, I will upgrade the TypeScript version

@aiday-mar
Copy link
Contributor

Here is the list of feedback I received. I will update this list as more feedback is received:

  • Make the +/- button stick to the bottom of the hover part in such a way that it is always visible. The +/- buttons would scroll with the scrollbar and would clip to the top or the bottom of their respective hover parts.
  • Increase +/- button size

@aiday-mar aiday-mar modified the milestones: November 2024, Backlog Dec 3, 2024
@connor4312 connor4312 added the feature-request Request for new features or functionality label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor-hover Editor mouse hover feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

No branches or pull requests

5 participants