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

5.1 - Restore command helper tests and add BannerHelper #17686

Merged
merged 7 commits into from
May 20, 2024
Merged

Conversation

markstory
Copy link
Member

When adding BannerHelper I noticed that we were missing test cases for the other helpers. It seems like the files were removed during 5.x, I have to assume during one of the many very large merge conflicts. I've restored those tests now.

These changes also contain a new console helper for emitting banners. With my terminal theme the following code

$styles = ['info.bg', 'warning.bg', 'error.bg', 'success.bg'];
$helper = new BannerHelper(new ConsoleIo());
$helper->withPadding(5);
foreach ($styles as $style) {
    $helper
        ->withStyle($style)
        ->output(['All done!']);

}

Emits the following.

Screenshot from 2024-05-16 23-38-43

I would appreciate any other results, as I'm not sure I have the colors right.

@markstory markstory added this to the 5.1.0 milestone May 17, 2024
'info' => ['text' => 'cyan'],
'info.bg' => ['background' => 'cyan', 'text' => 'black'],
Copy link
Member

Choose a reason for hiding this comment

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

A bright color for info would look like a result to me. Could we try grey with cyan text?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have grey right now, as we only do the basic 8 color ansi palette. We could use white as the background, or not include a banner style for info 🤷

Copy link
Member

Choose a reason for hiding this comment

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

White would work.

phpstan.neon.dist Outdated Show resolved Hide resolved

$lines = [
'',
$start . str_repeat(' ', $bannerLength) . $end,
Copy link
Member

Choose a reason for hiding this comment

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

We are always assuming utf-8 for multi-byte encoding, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, if folks need a non-utf8 encoding they'll have to configure the global encoding.

Copy link
Member

Choose a reason for hiding this comment

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

Right. What I meant was we're injecting ascii characters into an mb stream, so we can only support utf8 output.

Copy link
Member

Choose a reason for hiding this comment

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

Just realized this is only for console. I don't think consoles support anything incompatible with ascii space.

- newer phpunit eats our deprecation warnings.
- Fix phpcs error and phpstan config warning
- Fix new type issues that phpstan found
- Update split repos phpstan config
A cyan background could be mistaken for green.
@markstory markstory merged commit 3e90034 into 5.next May 20, 2024
10 checks passed
@markstory markstory deleted the banner-helper branch May 20, 2024 15:41
markstory added a commit to cakephp/docs that referenced this pull request May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants