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

Adjust everforest to resemble original more closely #5866

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

basbebe
Copy link
Contributor

@basbebe basbebe commented Feb 7, 2023

Adjust palettes and assignments:

@the-mikedavis the-mikedavis added A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 7, 2023
@the-mikedavis
Copy link
Member

\cc @CptPotato what do you think?

@CptPotato
Copy link
Contributor

I'm fine with this change, didn't know the colors were changed at some point 👍

When I created the port I didn't follow the original theme super closely because I preferred this look. I'm a bit on the fence about which version I like better but for the "official" themes that are bundled with helix it makes sense to adhere more to the original, so I'd say go ahead.

@CptPotato
Copy link
Contributor

CptPotato commented Feb 7, 2023

Here are some nits, but feel free to disregard any (especially those that would deviate from the original theme again, I just thought about sharing my perspective):

I think modules and types having the same color and style makes them harder to discern

old:
image

this PR:
image

`let` is not actually orange in the original theme

original:
image
screenshot source

this PR:
image

Italic may look bad with some fonts

This is debatable and up to personal preference but I initially didn't include italic text because it can look pretty ugly depending on the font or terminal, like with Fira Code for example:
image

It also looks like there's more italic text in this PR than in the original in general. This includes types and comments for example. I actually think with a nice font this can look pretty neat (I'm using italic comments myself) but it's different from the original.


Lastly, I think one difficulty is that vim just highlights code differently in some cases, sometimes it's even inconsistent within vim (one screenshot shows a write!() call in green, but an assert_eq!() call using aqua for example).

That being said, I still stand by the opinion that following the original somewhate closely is desirable (since that is what most users would expect I guess). Though I would at least change the color of let bindings from orange to red before merging.

@basbebe
Copy link
Contributor Author

basbebe commented Feb 8, 2023

Thanks for the feedback.
While I used gruvbox for a long time I really like this theme now, given it is similar in its qualities but has a slightly brighter background (light version) and more vivid colors.

While the original vim theme has many small adjustments for different languages, I tried to stay with what is defined in the code for the general treesitter attributes (see everforest/everforest.vim at master · sainnhe/everforest) since those are also close to what helix uses.

Some but not all of the code is mirrored / documented in https://github.com/sainnhe/everforest/blob/master/palette.md

The vscode theme with the same name (same author!) seems to deviate a lot from the vim one.

I made a new commit (see below) which I could further tweak or squash depending on your feedback.

I think modules and types having the same color and style makes them harder to discern

I decided to stick with the original theme there at first which defines YellowItalic for both of those (using more nuanced definitions in some but not all languages).
My solution would be to use yellow without italics for types, which seems to also have been in the original until sainnhe/everforest@62408f6 (also resembled in the screenshots).

let is not actually orange in the original theme

Thanks, fixed that oversight

Italic may look bad with some fonts

I see how this leaves a lot of room for debate.
The original has optional support for italics (which also includes italics for comments) and I decided to go with them due to personal preference and having seen italics used in other helix themes.

Fira Code particularly will be problematic in this case since it doesn't have italics natively (?) and needs special setup in the terminal to produce "proper" italics (like derived from another font).

I don't know what your preferred solution is for italics:

  • Provide separate italics theme
  • Include italics
  • Omit italics

@CptPotato
Copy link
Contributor

CptPotato commented Feb 8, 2023

Thanks for elaborating!

My solution would be to use yellow without italics for types

Sounds like a good compromise to me 👍

I don't know what your preferred solution is for italics:

  • Provide separate italics theme
  • Include italics
  • Omit italics

I'd say keep the italic modifiers and don't bother creating variants. I don't see this "issue" as a reason to remove them. To enable/disable italics it's probably a better idea to provide an editor config option, similar to global theme overrides that have been proposed.

I made a new commit (see below) which I could further tweak or squash depending on your feedback.

There aren't any issues left that stick out to me, so I think this is good to go now.
Squashing can be done by the maintainer that merges the PR if I remember correctly.

Adjust palettes and assignments:

Color palettes of upstream everforest where tweaked since creation of this port:
- sainnhe/everforest#108
- sainnhe/everforest#109

These adjustments move the helix everforest theme closer to the dcocumented
 upstream vim theme
@basbebe
Copy link
Contributor Author

basbebe commented Apr 6, 2023

Rebased and added styling of inlay-hints and soft-wrap indicator

@the-mikedavis the-mikedavis merged commit 1421b67 into helix-editor:master Apr 12, 2023
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 S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants