-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fixed irregular spacing between list items #79388
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
Conversation
@@ -208,6 +211,7 @@ public void MarkLineBreak() | |||
else | |||
{ | |||
_pendingLineBreak = true; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary white space
@JoeRobich, @akhera99 PTAL |
New ClassifiedTextRun(ClassificationTypeNames.Text, "–"), | ||
New ClassifiedTextRun(ClassificationTypeNames.Text, "Item 2"))))) | ||
|
||
ToolTipAssert.EqualContent(expected, intellisenseQuickInfo.Item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this assert removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not intended, just fixed this in the new commit
@@ -333,8 +332,7 @@ strong text | |||
italic text | |||
underline text | |||
|
|||
• Item 1. | |||
• Item 2. | |||
• Item 1. • Item 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These test validate the comment markup generated for LSP requests. It looks like we need to update ProtocolConversions.GetDocumentationMarkupContent now that we are not always appending a NewLine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaning there should be no change to this test if we are preserving the behavior in VS Code and other LSP editors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fanominals how is this going? You figuring out how to update that doc to get these tests clean?
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
src/Features/Core/Portable/QuickInfo/Presentation/TaggedTextExtensions.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Test2/IntelliSense/IntellisenseQuickInfoBuilderTests_Lists.vb
Outdated
Show resolved
Hide resolved
…net/roslyn into dev/t-gosingh/fix-list-items
// Documentation formatting may add an extra LineBreak after ContainerEnd to separate | ||
// elements in LSP scenarios. During rendering, we consume this extra LineBreak to | ||
// prevent double spacing, since each line is already rendered on its own line. | ||
if (taggedTexts is [var head, .. var tail] && head.Tag == TextTags.LineBreak) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always allocate the tail collection, even if the head doesn't match the check. Please defer slicing the tail until inside the if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, just fixed!
@dotnet-policy-service agree |
fixed bug: #39500