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

Add single width left margin for completion popup #2728

Merged
merged 2 commits into from
Jun 26, 2022
Merged

Add single width left margin for completion popup #2728

merged 2 commits into from
Jun 26, 2022

Conversation

sudormrfbin
Copy link
Member

@sudormrfbin sudormrfbin commented Jun 9, 2022 •
edited
Loading

Adds a bit of padding to the left edge of the completion popup:

Before After
before after

@CptPotato
Copy link
Contributor

I like this change 👍

As a side note/proposal: The scroll bar always looked a little odd to me because it's rather thin and not aligned to the right. Maybe it's just me, but I'm more used to this style (coc.vim for example):

image

@sudormrfbin
Copy link
Member Author

The scroll bar was actually next on my list, along with adding a scroll bar for the doc popup :)

@sudormrfbin sudormrfbin mentioned this pull request Jun 11, 2022
Merged
@the-mikedavis
Copy link
Member

Hmm strangely I have a separate style for that left margin. I bet my theme is missing a scope 🤔

before

@sudormrfbin
Clear with ui.menu style before rendering menu
23bc196 
When rendering a completion popup, the popup component will clear
the area with ui.popup and then the menu component would draw over
it using a table component. We remove the left edge of the area
before passing it to the table component (so that it will be left
as padding), and the table component uses ui.menu as the style.
If ui.menu and ui.popup are different the left edge of the popup
will look different from the rest of the popup. We avoid this by
clearing the whole area with ui.menu in Menu::render
@sudormrfbin
Copy link
Member Author

sudormrfbin commented Jun 21, 2022 •
edited
Loading

Fixed in 23bc196; the bug was from the area not being cleared by ui.menu and being splashed with ui.popup. So the margin color in the screenshot is ui.popup and the rest of the popup is ui.menu. You don't have any missing scopes :)

@the-mikedavis
Copy link
Member

Ah that's it! Just pulled down that commit and it's looking perfect.

the-mikedavis
the-mikedavis approved these changes Jun 21, 2022
View reviewed changes
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Looks good, I like the extra space 🎨

@archseer
Copy link
Member

I'm not sure how I feel about the extra space but let's try it in master 👍🏻

@archseer archseer merged commit 16ccc7e into helix-editor:master Jun 26, 2022
@sudormrfbin sudormrfbin deleted the completion-popup-padding branch June 26, 2022 11:04
@the-mikedavis the-mikedavis mentioned this pull request Feb 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the-mikedavis the-mikedavis the-mikedavis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sudormrfbin @CptPotato @the-mikedavis @archseer