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.

Sign up for GitHub

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

Jump to bottom

remove extra margins on completion hovers with markdown #59786

Merged
merged 7 commits into from Oct 15, 2018
Merged

remove extra margins on completion hovers with markdown #59786

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
beaffac
remove extra margins on completion hovers with markdown
Oct 1, 2018
6eece65
fixed styles to consider docs with multiple paragraphts
Oct 4, 2018
61d331d
space above docs as padding (instead of margin)
Oct 8, 2018
e8edae5
space above docs as padding (instead of margin)
Oct 8, 2018
370a916
styles for markdown-docs suggest widget having header but no content
Oct 9, 2018
1e1fcda
merge css rules
Oct 12, 2018
8e458c6
add padding to non-empty span elements inside `.markdown-docs`
Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/vs/editor/contrib/suggest/media/suggest.css
View file
Open in desktop
Expand Up @@ -226,7 +226,7 @@
opacity: 0.7;
word-break: break-all;
margin: 0;
padding: 4px 0 4px 5px;
padding: 4px 0 12px 5px;
}

.monaco-editor .suggest-widget .details > .monaco-scrollable-element > .body > .docs {
Expand All @@ -236,9 +236,23 @@
}

.monaco-editor .suggest-widget .details > .monaco-scrollable-element > .body > .docs.markdown-docs {
padding: 0;
white-space: initial;
}

.monaco-editor .suggest-widget .details > .monaco-scrollable-element > .body > .docs.markdown-docs > div,
.monaco-editor .suggest-widget .details > .monaco-scrollable-element > .body > .docs.markdown-docs > span:not(:empty) {
padding: 4px 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Last question :)

Why are we moving the padding from .docs.markdown-docs to .docs.markdown-docs > div?

Copy link
Contributor Author

@agurod42 agurod42 Oct 12, 2018 •
edited

Choose a reason for hiding this comment

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

Hehe, no problem!

I'll try to explain it:

Problem:

  1. We need this padding-bottom in order to provide space between the widget's header and content:

screen shot 2018-10-11 at 11 46 03 pm

  1. We also need the .markdown-docs padding to provide space between the content and the widget borders:

screen shot 2018-10-11 at 11 46 13 pm

  1. When there is no content to show, the .markdown-docs padding generates this unwanted effect:

Solution:

When the suggestion docs has markdown content, this function is being called in order to render it's contents:

https://github.com/Microsoft/vscode/blob/e8edae54aa99d06ddb37211bebc6faffb20ff1fb/src/vs/editor/contrib/suggest/suggestWidget.ts#L262

This one ends up calling the createElement in the htmlContentRenderer class, which eventually generates the html element that will be added as the .markdown-docs children:

https://github.com/Microsoft/vscode/blob/e8edae54aa99d06ddb37211bebc6faffb20ff1fb/src/vs/base/browser/htmlContentRenderer.ts#L30

When there is no content to show, the options.inline property becomes true and an empty span is added to the .markdown-docs. Here we shouldn't add padding.

In order to consider this scenario and avoid adding the problematic padding we can apply the padding to the .markdown-docs's immediate children instead of applying it to the .markdown-docs element itself. To do that we need to differentiate two cases:

  1. the children is a div element and we should always add padding
  2. the children is a span element and we should only add padding it the span is not empty (I didn't found any suggestion widget with markdown-docs and a non-empty span as children but I think we should handle it, just in case).

That's why I moved the padding from .docs.markdown-docs to .docs.markdown-docs > div (and also added another rule to handle the span:non(:empty) case).

Hope I was able to explain myself in a clear way.

Kr

}

.monaco-editor .suggest-widget .details > .monaco-scrollable-element > .body > .docs.markdown-docs > div > p:first-child {
margin-top: 0;
}

.monaco-editor .suggest-widget .details > .monaco-scrollable-element > .body > .docs.markdown-docs > div > p:last-child {
Copy link

@soleo soleo Oct 9, 2018 •
edited

Choose a reason for hiding this comment

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

Just curious, why do you need to use a specific rule like
.docs.markdown-docs > div > p:last-child? isn't it sufficient enough to have Just curious, why do you need to use a specific rule like .docs.markdown-docs p:last-child?

Copy link
Contributor

Choose a reason for hiding this comment

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

@soleo I think there was some copy/paste problem with your comment above

margin-bottom: 0;
}

.monaco-editor .suggest-widget .details > .monaco-scrollable-element > .body > .docs .code {
white-space: pre-wrap;
word-wrap: break-word;
Expand Down