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

Add option to change the newlines before and after the menu is drawn #280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zhigalin
Copy link

This feature is useful because it allows you to either reduce or set to zero the vertical margins in cases of a small terminal window (my case for which I made this feature) or a very large menu and you have to use every inch to fit it into the terminal,
or you can increment it to move or center the menu vertically.

@AydinHassan
Copy link
Member

What is the difference between this and the top/bottom padding?

@Zhigalin
Copy link
Author

Zhigalin commented Dec 18, 2023

What is the difference between this and the top/bottom padding?

If you want to decrement the number of lines the difference is that these 4 lines are not part of the padding number and as such cannot be removed and the space occupied by these lines cannot be used even if we set the padding to 0.

If you want to increment it the difference is in the color.
In the screenshot to the left we use the padding and to the right we use the new setting to align the menu vertically
Screenshot_term

@Zhigalin
Copy link
Author

@AydinHassan could you take another look at this PR?
Thank you

@AydinHassan
Copy link
Member

@Zhigalin sorry i took so long to look. I think it would make a bit more sense to just use the margin value here?

@Zhigalin
Copy link
Author

@Zhigalin sorry i took so long to look. I think it would make a bit more sense to just use the margin value here?

@AydinHassan margins are horisontal, the proposed fearure is abount vertical spacing.
Maybe I should rename it to verticalMargins or something like that for clarity?

@AydinHassan
Copy link
Member

yes but we have the concept of top/bottom padding and left/right padding so we could do the same with margin, and then use the top/bottom margin instead of the top/bottom frame count. Then we could also take in to consideration the margin auto option and center the menu vertically when it's set.

So yeah first lets add setMarginTopBottom and use that instead of the top/bottom frame lines stuff.

@AydinHassan
Copy link
Member

I was thinking something like:

[$marginTop, $marginBottom] = $this->style->getMarginTopBottom($frame->count());
      
$frame->prependNewLine($marginTop);
$frame->newLine($marginBottom);

in CliMenu::draw

and

/**
     * @return array{0: int, 1: int}
     */
    public function getMarginTopBottom(int $contentHeight): array
    {
        if ($this->isMarginAuto()) {
            $availableHeight = $this->terminal->getHeight();

            if ($contentHeight < $availableHeight) {
                return [
                    (int) floor(($availableHeight - $contentHeight) / 2),
                    (int) ceil(($availableHeight - $contentHeight) / 2)
                ];
            }

            return [0, 0];
        }

        return [$this->marginTop, $this->marginBottom];
    }

in MenuStyle of course with all the setters and props etc

@Zhigalin
Copy link
Author

Ok, will do

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.

2 participants