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

Improve the formatting of disabled menu items in different terminals #236

Merged
merged 5 commits into from
Apr 19, 2020

Conversation

jackwh
Copy link
Contributor

@jackwh jackwh commented Apr 18, 2020

I spent ages wondering why my disabled menu item didn't look like it was disabled, and eventually I figured out it's just that some Terminals don't support some of the less common ANSI escape sequences.

In my case, I was using PhpStorm's built-in Terminal window — which is obviously going to be a pretty common choice for users of this package — so here's a PR for a workaround.

  • Disabled items are now modified to use the ANSI "bright" equivalent of the same colour as their foreground text
  • To get a bright ANSI colour, just add 60 to an existing colour code
  • This bright effect makes both black and white text look greyed-out, and works well with other colours too
  • The bright text is then wrapped in the original dim modifier, to enhance the effect further in Terminals that do support dimming

Whilst I was researching what was going wrong, I found this list comparing support for the blink effect across different popular terminals. Though it's obviously for a different effect it does go to show you can't expect every terminal to support dimming.

Disabled items are now modified to use the ANSI "bright" equivalent of the same colour as their foreground text, as well as the dim escape sequence modifier.
@AydinHassan
Copy link
Member

hi @jackwh thanks for the PR!

Could you maybe add a screenshot so we can see how it looks?

Also looks like you need to update a test + some return types

@codecov-io
Copy link

codecov-io commented Apr 19, 2020

Codecov Report

Merging #236 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #236      +/-   ##
============================================
+ Coverage     92.87%   92.91%   +0.04%     
- Complexity      623      629       +6     
============================================
  Files            37       37              
  Lines          1866     1877      +11     
============================================
+ Hits           1733     1744      +11     
  Misses          133      133              
Impacted Files Coverage Δ Complexity Δ
src/MenuStyle.php 98.94% <100.00%> (+0.04%) 83.00 <9.00> (+6.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c48a4d8...734431e. Read the comment docs.

@jackwh
Copy link
Contributor Author

jackwh commented Apr 19, 2020

@AydinHassan Thanks, I've fixed the return types and the broken test.

Here's some screenshots — the first one is before any changes were made, and you can see there's no dim formatting on the PhpStorm window (on the right — macOS on the left). The rest are after the changes.

It's not perfect, but without a terminal supporting dimming, it's better than what was there before I think. Of course, ultimately its effectiveness will depend on the ANSI colours defined in the user's Terminal colourscheme.

Before:

Screenshot 2020-04-19 at 12 43 08

After:

Screenshot 2020-04-19 at 12 45 03

Screenshot 2020-04-19 at 12 45 28

Screenshot 2020-04-19 at 12 45 53

@AydinHassan
Copy link
Member

AydinHassan commented Apr 19, 2020

Thank you @jackwh looks great to me and thanks for the accompanying screenshots, that makes it super easy for me to check and approve! I'm happy to merge after the last question regarding the test has been resolved :)

@jackwh
Copy link
Contributor Author

jackwh commented Apr 19, 2020

Thanks, happy to contribute! It's a great package, thank you for releasing it!

@AydinHassan AydinHassan merged commit 44815c8 into php-school:master Apr 19, 2020
@AydinHassan
Copy link
Member

@jackwh merged and thanks for contributing!

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.

3 participants