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

Hover: jsdoc portion should wrap to ~80-100 chars #69388

Closed
Tyriar opened this issue Feb 25, 2019 · 10 comments
Closed

Hover: jsdoc portion should wrap to ~80-100 chars #69388

Tyriar opened this issue Feb 25, 2019 · 10 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug editor-hover Editor mouse hover verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Feb 25, 2019

#69204

Version: 1.32.0-insider (user setup)
Commit: 393b48d
Date: 2019-02-25T01:34:48.785Z
Electron: 3.1.3
Chrome: 66.0.3359.181
Node.js: 10.2.0
V8: 6.6.346.32
OS: Windows_NT x64 10.0.18841

image

It would be easier to read if wrapped somewhere around here (or a little more):

image

Maybe this should only apply the markdown sections that are not fenced code blocks.

@sandy081
Copy link
Member

I do not think hover should do the wrapping and should show the hover as it is given by the extension. It is the responsibility of the extension to preserve the wrapping provided by the user while writing the doc.

@mjbvz May I know if you are preserving the format of the js doc?

@mjbvz
Copy link
Contributor

mjbvz commented Feb 25, 2019

JS/TS does not wrap text documentation text and also does not know the size of the hover

Users can manually wrap jsdoc text if they wish (it is treated as markdown content so just end the line with a double space to hard wrap a line)

@sandy081
Copy link
Member

sandy081 commented Feb 26, 2019

I did not mean JS/TS server to wrap the text. I would expect they should preserve the format of the doc provided by the user.

I checked that JS/TS is preserving the format but the markdown renderer is not showing the new lines. For eg JS/TS sends following markdown contents with new line characters at end of each line

"Represents a reference to a command. Provides a title which
will be used to represent a command in the UI and, optionally,
an array of arguments which will be passed to the command handler
function when invoked."

But the markdown renderer is rendering it without respecting the new line characters.

image

It was true even in stable that hover is not respecting the new line chars

image

@jrieken Do you have any idea?

@sandy081 sandy081 added editor-hover Editor mouse hover bug Issue identified by VS Code Team member as probable bug labels Feb 26, 2019
@sandy081 sandy081 modified the milestone: February 2019 Feb 26, 2019
@jrieken
Copy link
Member

jrieken commented Feb 26, 2019

@jrieken Do you have any idea?

About the wrapping? To my knowledge that's how markdown works. You need two line breaks to get a visible line break

@sandy081
Copy link
Member

@mjbvz So new lines are missing when getting converted from a string to mark down. Can you please make sure new lines get preserved?

@sandy081 sandy081 added this to the February 2019 milestone Feb 26, 2019
@mjbvz
Copy link
Contributor

mjbvz commented Feb 26, 2019

@sandy081 This wrapping behavior is by design for markdown and I think it makes sense for documentation:

  • Most users are not intentionally hard wrapping their docs so that text is broken up a specific way, they add the line breaks so that the lines are not too long in the source
  • It leaves the hover widget to layout the text however it thinks would be be best instead of relying on the user to do break lines.
  • If the user hard wraps at a weird position—such as column 160 or column 40—and we preserved the breaks, the hover may end up looking really bad

As @jrieken mentioned, you can add a hard wrap by ending a line with two spaces.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 26, 2019

@sandy081 markdown is working correctly imo, the wrapping has to be done by the hover as wrapping points are different depending on the font family, size, etc. The fix for this probably looks something like this:

.hover .markdown {
  max-width: Xpx
}
.hover .markdown .code {
  /* break out of container in css or JS*/
}

@sandy081
Copy link
Member

I see different tools take different approaches here. For eg., Sublime wraps as per how user wrapping.

If the hover decides to wrap, then it should have same wrapping for all hovers. Otherwise the hovers look inconsistent. For eg., if we wrap TS/JS doc hovers at smaller width and leave others (like problems or hovers with code) with larger width it will look as below:

image

@misolori FYI

@sandy081
Copy link
Member

I think I liked this solution after playing around a bit. Here are some examples that show before (no wrapping) and after (with wrapping)

  • Before
    image

  • After
    image


  • Before
    image

  • After
    image


  • Before
    image

  • After
    image

@sandy081
Copy link
Member

Fix is to have a smaller max width (500px) for hovers not containing code except for problem hovers.

@RMacfarlane RMacfarlane added the verified Verification succeeded label Feb 27, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Apr 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug editor-hover Editor mouse hover verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants