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

theme: Enable style modifiers in theme.toml #113

Merged
merged 11 commits into from
Jun 6, 2021

Conversation

intarga
Copy link
Contributor

@intarga intarga commented Jun 4, 2021

Support for configuring style modifiers like bold, italic, etc. in theme.toml. They're done as an array, alongside fg and bg.

@intarga
Copy link
Contributor Author

intarga commented Jun 4, 2021

Seems to work. Going to write some documentation for theme.toml.

@archseer archseer marked this pull request as ready for review June 4, 2021 14:19
@archseer
Copy link
Member

archseer commented Jun 4, 2021

(Sorry, tapped the wrong button, meant to approve the CI run but it got unmarked as a draft)

helix-view/src/theme.rs Outdated Show resolved Hide resolved
@archseer
Copy link
Member

archseer commented Jun 4, 2021

This looks pretty good! Would you mind adding a short test for it? I know theme.rs doesn't contain any tests yet, but I think it would be useful to get some coverage

helix-view/src/theme.rs Outdated Show resolved Hide resolved
helix-view/src/theme.rs Outdated Show resolved Hide resolved
@intarga
Copy link
Contributor Author

intarga commented Jun 4, 2021

Perhaps we can add a contrib/themes directory for user submitted themes? Or make a separate repo for themes under helix-editor and link to it in the docs?

@intarga
Copy link
Contributor Author

intarga commented Jun 4, 2021

This looks pretty good! Would you mind adding a short test for it? I know theme.rs doesn't contain any tests yet, but I think it would be useful to get some coverage

Are these what you had in mind?

@archseer
Copy link
Member

archseer commented Jun 4, 2021

Yep, that looks fine! I'll accept a contrib/themes directory too if you want to add your theme?

@intarga intarga marked this pull request as draft June 4, 2021 23:36
@intarga intarga marked this pull request as ready for review June 5, 2021 08:15
@intarga intarga requested a review from pickfire June 5, 2021 08:16
book/src/configuration.md Outdated Show resolved Hide resolved
book/src/configuration.md Outdated Show resolved Hide resolved
book/src/configuration.md Outdated Show resolved Hide resolved
book/src/configuration.md Outdated Show resolved Hide resolved
book/src/configuration.md Outdated Show resolved Hide resolved
helix-view/src/theme.rs Outdated Show resolved Hide resolved
contrib/themes/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, there are tests and the code is better. The theme looks nice too.

Just wondering where you get inspiration for this theme?

book/src/configuration.md Outdated Show resolved Hide resolved
@intarga
Copy link
Contributor Author

intarga commented Jun 5, 2021

Just wondering where you get inspiration for this theme?

The colours mostly came from here: https://github.com/alnj/dotfiles
I use it as my terminal theme, and I just tried to get helix looking as close as I could to the neovim setup I'm used too.

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me except the toml key thing. I think rust version looks nicer. See what @archseer say.

@archseer
Copy link
Member

archseer commented Jun 6, 2021

I think bare keys are preferred, I just quoted all of my keys originally when I was porting my theme over. It's worth mentioning that key names with . need quoting though, or people will run into issues with their rule not being applied

@@ -28,3 +28,4 @@ slotmap = "1"

serde = { version = "1.0", features = ["derive"] }
toml = "0.5"
log = "~0.4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sidenote: @pickfire should we simply re-export log from helix_core?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary. We can just do this, it seemed better. For log it's easy to keep them in sync.

@pickfire
Copy link
Contributor

pickfire commented Jun 6, 2021

I think bare keys are preferred, I just quoted all of my keys originally when I was porting my theme over. It's worth mentioning that key names with . need quoting though, or people will run into issues with their rule not being applied

I think we can do something like the following for those with . right?

[ui]
bg = ...

@archseer
Copy link
Member

archseer commented Jun 6, 2021

No, it needs to be a top level string key:

{ "ui.menu" = <>} vs { ui = { menu = <>}}

Not quoting the key will result in a nested map and won't work correctly

@intarga intarga requested a review from archseer June 6, 2021 10:29
@pickfire
Copy link
Contributor

pickfire commented Jun 6, 2021

Nice, there are tests and the code is better. The theme looks nice too.

Just wondering where you get inspiration for this theme? I think this might get others confused why their stuff didn't get applied if they forgot to do the "x.x" quote.

@archseer
Copy link
Member

archseer commented Jun 6, 2021

Great work on this, especially the documentation part and congrats on being the first to contribute a theme! 🎉

@archseer archseer merged commit 54f3548 into helix-editor:master Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants