microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.05k stars 29.22k forks source link

Test and iterate on TS expandable hover apis #232685

Open mjbvz opened 6 days ago

mjbvz commented 6 days ago

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

aiday-mar commented 6 days ago

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:

https://github.com/user-attachments/assets/40e5a47f-bf12-4f2a-b8bf-ef6f31384b8f

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 commented 6 days ago

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 commented 5 days ago

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 commented 2 days ago

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?