Add Alternating Colors for Help Table #466
Conversation
spotify_player/src/ui/page.rs
Outdated
| Row::new(vec![ | ||
| Cell::from(format!("{command:?}")), | ||
| Cell::from(format!("[{keys}]")), | ||
| Cell::from(command.desc()), | ||
| ]) | ||
| // adding alternating row colors | ||
| .style(if (i + scroll_offset) % 2 == 0 { | ||
| ui.theme.app().bg(Color::DarkGray) |
There was a problem hiding this comment.
should be a component style: https://github.com/aome510/spotify-player/blob/master/docs/config.md#component-styles
There was a problem hiding this comment.
Did you have a particular style in mind? Almost seems to me like having a new alternate row field for the ComponentStyle struct could work.
There was a problem hiding this comment.
yeah, I think you could add new component style named help_table_alternating_row for example. DarkGray background might be a good start. You can also try different modifiers; I'm not sure how nice they will look. It's to allow people with incompatible theme to update the style.
| @@ -74,6 +77,7 @@ pub struct ComponentStyle { | |||
| pub playlist_desc: Option<Style>, | |||
| pub table_header: Option<Style>, | |||
| pub selection: Option<Style>, | |||
| pub secondary_row: Option<Style>, | |||
There was a problem hiding this comment.
I named this secondary row because I thought it could be used in more places than just the help table, but let me know if it should just be for the help table.
spotify_player/src/config/theme.rs
Outdated
| @@ -58,6 +58,9 @@ pub struct Palette { | |||
| pub bright_blue: Color, | |||
| #[serde(default = "Color::bright_yellow")] | |||
| pub bright_yellow: Color, | |||
|
|
|||
| #[serde(default = "Color::dark_gray")] | |||
| pub dark_gray: Color, | |||
There was a problem hiding this comment.
I added support for dark gray coloring, but is there a better way to add support for coloring modifications? Dark and Bright colors seem to have some repetition. And should other dark colors be added as well?
There was a problem hiding this comment.
No you shouldn't add new field to the palette, it represents the basic 16-color palette with additional fields for background and foreground.
spotify_player/src/config/theme.rs
Outdated
| pub fn secondary_row(&self) -> tui::style::Style { | ||
| match &self.component_style.secondary_row { | ||
| None => Style::default() | ||
| .bg(StyleColor::DarkGray) |
There was a problem hiding this comment.
can use StyleColor::Rgb instead if you want a specific color that is not available in the palette. My suggestion would be using default theme if None, so it doesn't cause any surprise for current users.
There was a problem hiding this comment.
Correct me if I'm wrong, but this enables the alternating row colors by default, right? I was thinking that should be the intended behavior; should it instead only set alternating row colors when using a custom theme?
There was a problem hiding this comment.
I was thinking that should be the intended behavior; should it instead only set alternating row colors when using a custom theme?
Yeah, it should be opt-in. My reasoning is that setting to some default style (e.g dark gray as above) might "break" some users' theme. If one wants the alternating colors, they can update the style in their configs.
…fy-player into alternating_help_table_colors
examples/theme.toml
Outdated
| @@ -42,6 +42,31 @@ bright_magenta = "#ff79c6" | |||
| bright_cyan = "#8be9fd" | |||
| bright_white = "#ffffff" | |||
|
|
|||
| [[themes]] | |||
| name = "my_dracula" | |||
There was a problem hiding this comment.
nit: can you name it dracula2 or something? Can also add a small comment explaining the difference between this and the previous dracula
|
Why not introduce this to the music page as well? |
Do you mean track table? Should be easy to add tho |
I started using spotify-player, and I found the help table difficult to read. I added alternating row coloring as pictured below to help with identifying keybindings.