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

remove extra margins on completion hovers with markdown #59786

Merged
merged 7 commits into from Oct 15, 2018

Conversation

Projects
None yet
4 participants
@agurodriguez
Contributor

agurodriguez commented Oct 1, 2018

Fixes #59036

@ramya-rao-a

We need those margins for docs that have multiple paragraphs.

Below is one such case:

image

Below is the same case with the changes in this PR

image

Set sticky to true. Then in the local instance of VS Code, run Help -> Toggle Developer Tools -> Elements`

So, we have to

  • Remove the margin top only on the first <p>, and margin bottom on the last <p>
  • Add margin-bottom to the element above the docs
@agurodriguez

This comment has been minimized.

Contributor

agurodriguez commented Oct 4, 2018

Hi @ramya-rao-a,

Loved the sticky trick. Thank you!

I made the changes you requested:

screen shot 2018-10-04 at 10 32 24 am

screen shot 2018-10-04 at 10 32 34 am

screen shot 2018-10-04 at 10 32 40 am

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Oct 8, 2018

Well... now the bottom is clipped off

image

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Oct 8, 2018

Now there is no margin between paragraphs when there are only 2 paragraphs..

Before:

image

After:

image

Instead of setting margin:0 on the first and last child, try setting margin-top:0 on the first child and margin-bottom:0 on the last child.

@agurodriguez agurodriguez force-pushed the agurodriguez:vscode-59036 branch from 61d331d to e8edae5 Oct 8, 2018

@agurodriguez

This comment has been minimized.

Contributor

agurodriguez commented Oct 8, 2018

Well... It was harder than it seemed 😅

Now I think it working as expected:

header + one paragraph:

screen shot 2018-10-08 at 4 37 14 pm

header + two paragraphs:

screen shot 2018-10-08 at 4 37 34 pm

header + more than two paragraphs

screen shot 2018-10-08 at 4 37 03 pm

header only

screen shot 2018-10-08 at 4 36 45 pm

no header, no markdown

screen shot 2018-10-08 at 4 38 00 pm

no header, markdown

screen shot 2018-10-08 at 4 38 05 pm

I was forced to add a class when rendering the widget in order to be able to remove the header padding when the docs section is empty. I think it shouldn't be a problem.

Thanks for your help!

@@ -264,6 +264,8 @@ class SuggestionDetails {
this.docs.appendChild(renderedContents.element);
}
toggleClass(this.el, 'empty-docs', this.docs.innerText.length === 0);

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Oct 8, 2018

Member

Is the assumption that this.docs.innerText is empty when markdown is being used?

This comment has been minimized.

@agurodriguez

agurodriguez Oct 8, 2018

Contributor

Yes. this.docs refers to the p element which eventually has the markdown-docs class. It's innerText property will be empty if there is no content to show (Just like in the case shown in the image below).

screen shot 2018-10-08 at 5 26 50 pm

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Oct 9, 2018

Member

I am hesitant to add this class. On a cursory read of the code, it would lead one to believe that there are no "docs" associated with the suggestion item. In the suggestWidget file docs is referring to the part that appears on the right of the suggestion list.

I actually liked the change you did in your previous commit of increasing the bottom padding of .type all the time. Wouldnt that be enough (plus the margin-top/margin-bottom on first/last child)?

This comment has been minimized.

@agurodriguez

agurodriguez Oct 9, 2018

Contributor

Yeahp. You're right.

The problem with increasing the padding of .type all the time arises when the .docs item doesn't have content:

screen shot 2018-10-09 at 12 49 42 am

Here the space below the header should be smaller. See current (v1.28.0-431ef9d) aspect:

screen shot 2018-10-09 at 12 58 24 am

I think I just found an approach that doesn't need the empty-docs class.

This comment has been minimized.

@agurodriguez

agurodriguez Oct 11, 2018

Contributor

@ramya-rao-a I think the last commit solves this last issue without adding the empty-docs class. All the cases detailed in this comment #59786 (comment) are still working.

PS. Do you mind if I squash the commits in this PR?

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

This comment has been minimized.

@soleo

soleo Oct 9, 2018

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?

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Oct 9, 2018

Member

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

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

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Oct 12, 2018

Member

Last question :)

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

This comment has been minimized.

@agurodriguez

agurodriguez Oct 12, 2018

Contributor

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:

const renderedContents = this.markdownRenderer.render(item.suggestion.documentation);

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:

function createElement(options: RenderOptions): HTMLElement {

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

@ramya-rao-a

Thanks @agurodriguez!
I hope you registered for the Microsoft Hactoberfest :)

@ramya-rao-a ramya-rao-a merged commit 3266781 into Microsoft:master Oct 15, 2018

2 checks passed

VS Code #20181012.9 succeeded with issues
Details
license/cla All CLA requirements met.
Details

@ramya-rao-a ramya-rao-a added this to the October 2018 milestone Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment