feat: add per-mode theme overrides - #2324
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe theme configuration now supports light- and dark-appearance custom color overrides. Automatic theme switching applies active mode overrides after shared custom and legacy accent colors. Tests and documentation cover the new behavior. ChangesTheme appearance overrides
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HostAppearance
participant resolve_effective_theme
participant resolve_palette_for_theme_name
participant Palette
HostAppearance->>resolve_effective_theme: select light or dark appearance
resolve_effective_theme->>resolve_palette_for_theme_name: pass active mode overrides
resolve_palette_for_theme_name->>Palette: apply shared custom colors
Palette->>Palette: apply active mode-specific colors
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hi @aneym, thanks for your interest in contributing! Herdr automatically admits focused bug fixes from contributors who are not maintainers when the title uses Contributors who are not maintainers may submit only focused bug fixes. If this pull request fixes a bug, rename it to use a conventional Feature requests, behavior changes, and other proposals belong in GitHub Discussions and require maintainer approval before a pull request. If this gate classified the pull request incorrectly, reply and tag a maintainer listed in Patch size: 7 changed files, 391 changed lines. See https://github.com/herdrdev/herdr/blob/master/CONTRIBUTING.md for the contribution policy. |
Greptile SummaryThe PR adds light- and dark-specific custom palette overrides used when automatic theme switching is enabled.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/config/theme.rs | Adds deserializable light and dark color override blocks with parsing coverage. |
| src/app/mod.rs | Selects and applies the resolved appearance-specific override after shared palette customization. |
| src/app/state.rs | Adds token-by-token application of appearance-specific palette overrides. |
| docs/next/website/src/data/config-reference.json | Documents every supported light and dark override key. |
| docs/next/website/src/content/docs/configuration.mdx | Documents the new blocks and their resolution precedence. |
Reviews (3): Last reviewed commit: "feat: add per-mode theme overrides" | Re-trigger Greptile
66ee3b8 to
d1a4e65
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/theme.rs (1)
126-146: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
sidebar_bgto mode-specific overrides.
ModeThemeColorsomitssidebar_bg, althoughCustomThemeColorsandPalettesupport it. A user cannot set a light-only or dark-only sidebar background.Add
sidebar_bgtoModeThemeColors. Apply it inPalette::with_mode_overrides. Add parsing and palette-resolution coverage.Proposed fix
--- a/src/config/theme.rs +++ b/src/config/theme.rs pub struct ModeThemeColors { pub accent: Option<String>, pub panel_bg: Option<String>, + pub sidebar_bg: Option<String>, pub surface0: Option<String>,--- a/src/app/state.rs +++ b/src/app/state.rs if let Some(c) = &custom.panel_bg { self.panel_bg = parse_color(c); } + if let Some(c) = &custom.sidebar_bg { + self.sidebar_bg = parse_color(c); + }Based on supplied downstream context,
src/app/state.rs:641-692has the same omission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cefb19c-6dd0-416f-9a93-164b72966a75
📒 Files selected for processing (7)
docs/next/website/src/content/docs/configuration.mdxdocs/next/website/src/data/config-reference.jsonsrc/app/mod.rssrc/app/state.rssrc/config.rssrc/config/theme.rssrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/next/website/src/content/docs/configuration.mdx
- docs/next/website/src/data/config-reference.json
- src/main.rs
- src/app/state.rs
- src/app/mod.rs
- src/config.rs
allow [theme.custom.light] and [theme.custom.dark] blocks so custom overrides can differ per resolved appearance when auto_switch flips. precedence: theme defaults, then [theme.custom], then the block for the active mode. configs without the new blocks resolve unchanged. refs herdrdev#837
d1a4e65 to
baff68f
Compare
|
Added |
Summary
[theme.custom]applies one set of overrides in both modes, so withauto_switcha value tuned for the dark theme washes out when the light theme activates. #837 proposed per-mode blocks and @ogulcancelik approved a PR there.This adds
[theme.custom.light]and[theme.custom.dark]. Resolution order: theme defaults, then[theme.custom], then the block matching the resolved appearance. Configs that do not use the new blocks resolve exactly as they do today.Both panes are the same seeded layout rendered with
catppuccin-latteactive; the only difference is the[theme.custom.light]block.Refs #837. #2032 asks for the same capability from a full-theme-pair angle; this shape covers it through
[theme.custom.light]/[theme.custom.dark]alone.Test plan
just checksteps all pass: fmt, clippy, windows-lint, config-reference parity, integration assets, plugin marketplace, script unittestslive_handoffx2,api_pingpane-info) are pty-timing timeouts that fail identically on unmodified master in the same environmentsrc/config/theme.rscover parsing both blocks, per-mode resolution precedence, and that configs without the new blocks resolve unchangedauto_switchflipping between catppuccin-mocha and catppuccin-latte