add the ability to have an underline in a separate color than text #52
Conversation
| _ => { | ||
| break; | ||
| } | ||
| }, |
There was a problem hiding this comment.
According to https://en.wikipedia.org/wiki/ANSI_escape_code, 58 is a non-standard addition. But it seems to be supported in some terminals, so I'm okay with adding it. Should we support 59 as well (default underline color)? Similar to, say, 24 for disabling underlining.
There was a problem hiding this comment.
Agreed. I just added 59. Thanks.
|
Thank you! Just one minor comment.
Quite the contrary, the contributions are very much appreciated! |
|
For anyone following this issue in the future. I just want to add a bit of documentation here since some takes a bit to find. |
|
@sharkdp Would it be possible to get a published release of this by 8/15? Our next nushell release is 8/16. |
|
Done: https://github.com/sharkdp/lscolors/releases/tag/v0.12.0 This is v0.12.0 instead of v0.11.2 because the That's maybe something that should be improved using |
|
Thank you, sir. |
This PR augments the
Stylestruct to add the underline color as anOption<Color>. Previously, underline, which is ansi escape58, was not being parsed which was why the foreground color was being parsed as the underline color thereby making text blink on some terminals namely Mac's Terminal.app and Windows Terminal.So, now when one calls
to_crossterm_style(), if there is no underline set, underline will be set to None, and therefore not blink.I also augmented the tests in style.rs to test a underline style.
Sorry for all the PRs from the nushell team. We've just been trying to upgrade to crossterm 0.24 and found these issues. Hopefully this is the last hotfix we come across for underlines. I have tested this PR with nushell and it seems to be working fine now with these changes in Windows Terminal.