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

Fix hover when it should separate multiple summaries #8005

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Fix hover when it should separate multiple summaries #8005

merged 4 commits into from
Feb 24, 2023

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Dec 8, 2022 •
edited
Loading

Fixes #6998

Summary of the changes

The actual behavior:
IFzLurRJ0s

The expected behavior:
51VqKTRMin

The fix:
The changes in this PR helps to add a line of padding in between summaries.
image

It somehow seems like it is adding two new lines. So I need to figure our why this is happening.

Fixes: #6998

@maryamariyan maryamariyan requested a review from a team as a code owner December 8, 2022 20:14
@maryamariyan maryamariyan self-assigned this Dec 8, 2022
@DoctorKrolic
Copy link
Contributor

I expected the proper fix to be the thing, that was done in the legacy editor with the separation line, but this implementation is ok as a tactical fix. I believe, this separation line is just not available in the current state of LSP-based hovers, right?

@maryamariyan
Copy link
Member Author

maryamariyan commented Dec 8, 2022 •
edited
Loading

@DoctorKrolic Would be good to compare with the source code for the legacy editor.

I think the separation line is not available.

@maryamariyan
Copy link
Member Author

The double-lining is fixed now by not providing any content for the ClassifiedTextDocument

image

@DoctorKrolic
Copy link
Contributor

Would be good to compare with the source code for the legacy editor

I believe, the legacy editor is not open source. Since your are a Member and can push to this repo directly, you might probably have access to the internal VS repository as well, where the sources for the legacy editor are most likely to be. I don't think comparing source code is a valid thing to do here since these two editors are based on completely different approaches of making IDE tooling though

ryanbrandenburg
ryanbrandenburg approved these changes Dec 8, 2022
View reviewed changes
...src/Microsoft.AspNetCore.Razor.LanguageServer/Tooltip/DefaultVSLSPTagHelperTooltipFactory.cs
}
else
{
classifiedElementContainer.Add(new ContainerElement(ContainerElementStyle.Wrapped, new ClassifiedTextElement()));
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could get a separation line back, but from a quick look at the legacy code I'd guess it may not be compatible with LSP. Also the accessibility on the old separator is TERRIBLE in dark mode. I've got perfect vision (when corrected) and I almost don't notice that it exists, so I feel like it's not a huge deal if we don't get the separator back.

allisonchou
allisonchou reviewed Dec 8, 2022
View reviewed changes
Copy link
Contributor

@allisonchou allisonchou left a comment

Choose a reason for hiding this comment

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

Looks great! I feel like we should add tests to make sure we don't regress this in the future.

If the separation line is a popular ask, we can talk to the Editor team about adding support for it in the future. I don't think it should block this PR, though.

...src/Microsoft.AspNetCore.Razor.LanguageServer/Tooltip/DefaultVSLSPTagHelperTooltipFactory.cs Show resolved Hide resolved
davidwengier
davidwengier approved these changes Dec 10, 2022
View reviewed changes
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Nice work!
Just gonna echo everyone else and say "tests" :D

@maryamariyan
Copy link
Member Author

Thanks for the reviews @ryanbrandenburg and @davidwengier. Some existing tooltip tests are failing and I am gonna fix them into this PR

@maryamariyan
Copy link
Member Author

Fixed the two failing tests in DefaultVSLSPTagHelperTooltipFactoryTest.cs due to the fix in this PR.

Remaining test failures that might show up in CI would be flaky integration tests and unrelated to the change here.

@maryamariyan maryamariyan merged commit 0765857 into main Feb 24, 2023
@maryamariyan maryamariyan deleted the fix-hover branch February 24, 2023 00:11
@allisonchou allisonchou mentioned this pull request Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allisonchou allisonchou allisonchou left review comments

@davidwengier davidwengier davidwengier approved these changes

@ryanbrandenburg ryanbrandenburg ryanbrandenburg approved these changes

Assignees

@maryamariyan maryamariyan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hover should separate multiple summaries
5 participants
@maryamariyan @DoctorKrolic @davidwengier @ryanbrandenburg @allisonchou