Closed jackwh closed 4 years ago
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
Merging #236 into master will increase coverage by
0.04%
. The diff coverage is100.00%
.
@@ 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.
@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:
After:
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 :)
Thanks, happy to contribute! It's a great package, thank you for releasing it!
@JackWH merged and thanks for contributing!
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.
60
to an existing colour codeWhilst 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.