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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Show-Hints] Hint popup exceeds the viewport's height #6979

Closed
wants to merge 2 commits into from

Conversation

benjaminr-ps
Copy link
Contributor

The current implementation does not taking care of the top-position of the popup window while calculating the final height of it.

The current implementation does not taking care of the top-position of the popup window while calculating the final height of it.
@marijnh
Copy link
Member

marijnh commented Aug 26, 2022

That if seems to address precisely that — if the popup is higher then the window, it shrinks it. Adding box.top doesn't seem reasonable here.

@benjaminr-ps
Copy link
Contributor Author

No, it does not shrink it then, instead it will set the top position more to the top.

But therefore, the actual top position must be taken into account, here.

@benjaminr-ps
Copy link
Contributor Author

image

@marijnh
Copy link
Member

marijnh commented Aug 26, 2022

Right. But the branch you're modifying here is specifically for the case where the pop-up is itself higher than the screen, so just adding this without adjusting the rest of the logic is not a good idea.

But also, in your screenshots, the current behavior actually seems preferable — with the change, you can no longer see what you are typing because the pop-up covers it.

@benjaminr-ps
Copy link
Contributor Author

Ah, sorry, of course it looks proper to you.
Here is how it is actually rendered, exceeding the viewport:
grafik

@benjaminr-ps
Copy link
Contributor Author

Adapted your suggestion, and only resized the hint's popup window.

You are right, it looks much better, and keeps user-experience high ;)

@marijnh
Copy link
Member

marijnh commented Aug 26, 2022

That only covers the case where the pop-up is below the cursor ... and the existing logic seems seriously dodgy as well. Does attached patch work for you?

@benjaminr-ps
Copy link
Contributor Author

The current version was working fine in case of showing it above.

Nevertheless your implementation works great, and is using the full horizontal space, if needed, as well.

@benjaminr-ps
Copy link
Contributor Author

I will close this pull request, as it is overhauled now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants