feat: add sixteen colors theme #124
Conversation
supports light terminal background
|
Thanks for picking this up! This looks great; I've added some comments - apologies if they're a bit nitpicky, though I guess we live with anything we add here for a long time, so worth iterating a bit :-) Can you also please post new dark and light screenshots, ideally using both an ANSI terminal for the ANSI Sixteen, and the old Windows console for the System themes? Cheers! |
|
Thanks for taking the time to review :) but I don't see the comments, did you submit the review? I'll take the screenshots today 👍 |
ggobbe
force-pushed
the
feat/sixteen-colors-themes
branch
from
6e20c08 to
4749eb0
Compare
|
I've removed the System Sixteen theme, it looked ok when the command line was using a white background but not ok at all with a dark background, hence its name didn't make sense, I think it'd be better to add a System "light" theme in a later PR if someone requires it. Here are some screenshots of the ANSI Sixteen theme, both with light and dark background in Terminal Powershell, and in Command Line. Windows Terminal - Dark Background Windows Terminal - Light Background Windows Command Line |
README.md
Outdated
| @@ -43,9 +43,11 @@ The following built-in themes are available: | |||
| * `ConsoleTheme.None` - no styling | |||
| * `SystemConsoleTheme.Literate` - styled to replicate _Serilog.Sinks.Literate_, using the `System.Console` coloring modes supported on all Windows/.NET targets; **this is the default when no theme is specified** | |||
| * `SystemConsoleTheme.Grayscale` - a theme using only shades of gray, white, and black | |||
| * `AnsiConsoleTheme.Literate` - an ANSI 16-color version of the "literate" theme; we expect to update this to use 256-colors for a more refined look in future | |||
| * `SystemConsoleTheme.Sixteen` - a version of the "literate" theme that works with light and dark backgrounds | |||
There was a problem hiding this comment.
"a sixteen-color theme that works well with both light and dark backgrounds"?
There was a problem hiding this comment.
system sixteen theme removed
README.md
Outdated
| * `AnsiConsoleTheme.Grayscale` - an ANSI 256-color version of the "grayscale" theme | ||
| * `AnsiConsoleTheme.Code` - an ANSI 256-color Visual Studio Code-inspired theme | ||
| * `AnsiConsoleTheme.Sixteen` - an ANSI 16-color version of the "literate" theme that works with light and dark backgrounds |
There was a problem hiding this comment.
"an ANSI 16-color theme that works well with both light and dark backgrounds"?
| [ConsoleThemeStyle.LevelInformation] = BrightCyan, | ||
| [ConsoleThemeStyle.LevelWarning] = BrightYellow, | ||
| [ConsoleThemeStyle.LevelError] = BrightRed, | ||
| [ConsoleThemeStyle.LevelFatal] = "\x1b[38;5;0015m\x1b[48;5;0196m", |
There was a problem hiding this comment.
Perhaps we drop background coloring from the "Sixteen" themes?
There was a problem hiding this comment.
I made it BrightRed (same as Error level then)
| public static AnsiConsoleTheme Sixteen { get; } = new AnsiConsoleTheme( | ||
| new Dictionary<ConsoleThemeStyle, string> | ||
| { | ||
| [ConsoleThemeStyle.Text] = Reset, |
There was a problem hiding this comment.
We shouldn't need resets, here (though I understand why they might sometimes help) - can we define a constant Unthemed = "" for these purposes?
There was a problem hiding this comment.
I've replaced all resets by Unthemed 👍
| [ConsoleThemeStyle.LevelInformation] = new SystemConsoleThemeStyle { Foreground = ConsoleColor.Cyan }, | ||
| [ConsoleThemeStyle.LevelWarning] = new SystemConsoleThemeStyle { Foreground = ConsoleColor.Yellow }, | ||
| [ConsoleThemeStyle.LevelError] = new SystemConsoleThemeStyle { Foreground = ConsoleColor.Red, }, | ||
| [ConsoleThemeStyle.LevelFatal] = new SystemConsoleThemeStyle { Foreground = ConsoleColor.White, Background = ConsoleColor.Red }, |
There was a problem hiding this comment.
Perhaps we drop background coloring from Sixteen?
There was a problem hiding this comment.
system sixteen theme removed
| @@ -18,6 +18,27 @@ namespace Serilog.Sinks.SystemConsole.Themes | |||
| { | |||
| static class AnsiConsoleThemes | |||
| { | |||
| const string Reset = "\x1b[0m"; | |||
There was a problem hiding this comment.
Since (hopefully!) we might eventually use constants for the other ANSI themes, can we move these to a separate (static) class AnsiEscapeSequence? I.e. so that when we use them here they'd appear as AnsiEscapeSequence.Reset etc. This will avoid the current class becoming a bit bloated.
|
The screenshots look great! Agreed that removing the system version of the theme made sense. I've submitted my review now - sorry :-) ... Some of my comments will be outdated, please ignore where necessary. I'll try to loop back ASAP. Thanks again! |
|
Thanks for all the feedback, I've applied all suggestions (I think ^^). Here are screenshots on both light and dark background with these change applied (you were right, the reset wasn't necessary):
|
|
Wonderful - thank you! |
What issue does this PR address?
Current themes are not working on light terminal background.
Resolves #123
This PR builds on top of @stopdropandrew work in PR #118
As it looks like @stopdropandrew is away for a while, I took the liberty to start from his work and apply @nblumhardt's requests.
It adds a new theme
Sixteenthat is compatible with all the default unix terminal themes, mostly by avoiding using black/white colors and using the default more heavily.Sixteen:
Does this PR introduce a breaking change?
No breaking changes
Please check if the PR fulfills these requirements