Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Picker separator below input line lacks a theme key #2522

Closed
kyrime opened this issue May 20, 2022 · 3 comments · Fixed by #2523
Closed

Picker separator below input line lacks a theme key #2522

kyrime opened this issue May 20, 2022 · 3 comments · Fixed by #2523
Labels
A-theme Area: Theme and appearence related C-enhancement Category: Improvements

Comments

@kyrime
Copy link
Contributor

kyrime commented May 20, 2022

Currently, the picker separator is hardcoded to the default theme's comet color (purplish). The relevant code is at https://github.com/helix-editor/helix/blob/master/helix-term/src/ui/picker.rs#L590.

image

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-theme Area: Theme and appearence related labels May 20, 2022
@the-mikedavis
Copy link
Member

We should be able to replace that line:

let sep_style = Style::default().fg(Color::Rgb(90, 89, 119));

with something like:

let sep_style = cx.editor.theme.get("ui.text.separator");

and add the new scope to the default theme here:

helix/theme.toml

Lines 54 to 56 in e8e2526

"ui.text" = { fg = "lavender" }
"ui.text.focus" = { fg = "white" }
"ui.virtual" = { fg = "comet" }

as comet.

The scope name might need some bike-shedding. Would you like to open a PR?

@kyrime
Copy link
Contributor Author

kyrime commented May 20, 2022

The key name/scope is the reason I didn't open a PR, haha. Since the borders of the picker use ui.background as their key I was thinking it should be ui.background.separator to draw a reasonable default for all the other themes, but then I thought ui.background isn't the most descriptive name for borders in the first place and decided to just leave it to someone more familiar with the theme key names. I'll open a PR.

@the-mikedavis
Copy link
Member

Hmm yeah I'm not sure what the best scope name for this would be either 😅

What do others think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Theme and appearence related C-enhancement Category: Improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants