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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Put marker hovers on top #166560

Merged
merged 2 commits into from Feb 27, 2023
Merged

Put marker hovers on top #166560

merged 2 commits into from Feb 27, 2023

Conversation

azdavis
Copy link
Contributor

@azdavis azdavis commented Nov 17, 2022

Description

This puts marker hovers on top of all other hover kinds, and shifts down all hover kinds that used to be ahead of marker hovers by 1.

An effect of this is that diagnostic messages are now on top when hovering.

Before

before

After

after

Closes

Close #73120

Tests

馃憖

@azdavis
Copy link
Contributor Author

azdavis commented Nov 17, 2022

@microsoft-github-policy-service agree

@azdavis
Copy link
Contributor Author

azdavis commented Dec 7, 2022

@rebornix could this have a look?

@azdavis
Copy link
Contributor Author

azdavis commented Dec 7, 2022

some more examples of errors on top:

Millet

millet

rust-analyzer

ra

@alexdima alexdima assigned alexdima and unassigned rebornix Dec 10, 2022
@alexdima alexdima self-requested a review December 10, 2022 22:29
@AlpacaFur
Copy link

Hi @rzhao271 and @daviddossett, if you have a minute could you take a look at this PR? Based on the corresponding issue (#73120), this would fix a UX issue that many people have been struggling with for a while (opened 2019, >110 reactions, no negative comments). It also looks like the actual code changes are minimal, so this is primarily a design decision. Thank you both for your time!

rzhao271
rzhao271 previously approved these changes Dec 28, 2022
Copy link
Contributor

@rzhao271 rzhao271 left a comment

Choose a reason for hiding this comment

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

After this change is merged, for short hovers, if the hover is going downwards, then the error appears closer to the error squiggle, and if the hover is going upwards, then the error appears farther from the error squiggle.

For long hovers in either direction, because the user currently needs to scroll to the bottom of the hover just to see the error, the error appearing at the top instead, which would be provided by merging this PR, is good, because users wouldn't have to scroll anymore just to see the error message.

LGTM

I'll wait for @alexdima to provide a second approval, though.

@azdavis
Copy link
Contributor Author

azdavis commented Jan 10, 2023

Any progress on this?

@azdavis
Copy link
Contributor Author

azdavis commented Jan 26, 2023

@alexdima are you able to approve this?

@alexdima alexdima added this to the March 2023 milestone Feb 27, 2023
@aiday-mar aiday-mar merged commit e1967e9 into microsoft:main Feb 27, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-hover Editor mouse hover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

For long hovers, errors appear all the way at the bottom and require scrolling
7 participants