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: Add Tokyo Night variants #835

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

foster-hangdaan
Copy link

@foster-hangdaan foster-hangdaan commented Aug 17, 2024

This PR adds three variants of the Tokyo Night theme:

Closes: #827

Screenshots

Tokyo Night Light

image
image

Tokyo Night Storm

image
image

Tokyo Night Moon

image
image


  • I searched the issue tracker and this hasn't been PRed before.
  • My changes are not on the do-not-PR list for this project.
  • My commits conform to the git conventions.
  • My changes are visual; I've included before and after screenshots.
  • I am blindly checking these off.
  • Any relevant issues or PRs have been linked to.
  • This a draft PR; I need more time to finish it.

@i-am-linja
Copy link

Hey @foster-hangdaan, I hate to nitpick but I believe the NVim port has italic comments and keywords across all variants. I can't submit a PR to the PR so could you please slant them? I really like them.

@foster-hangdaan
Copy link
Author

Hey @foster-hangdaan, I hate to nitpick but I believe the NVim port has italic comments and keywords across all variants. I can't submit a PR to the PR so could you please slant them? I really like them.

The keywords and comments for the variants have been italicized. Note that I will not be modifying the original Night theme since it will be out of scope for this PR which already brings three new variants.

By explicitly setting them.
i-am-linja added a commit to i-am-linja/themes that referenced this pull request Aug 20, 2024
To match variants.
Ripped verbatim from doomemacs#835.
This applies a lighter font color for non-active line numbers for the
Tokyo Night Light theme. This helps make the active line number standout
more.
@azzamsa
Copy link

azzamsa commented Aug 21, 2024

But it looks very different than the upstream.

image

image

@i-am-linja
Copy link

But it looks very different than the upstream.

Worth noting: "upstream" is considered to be the VSCode theme, not the NVim port. Also, are you certain you're using the same variant in each app? Looks like Neovide is in Storm, whereas DOOM is in Night or Moon.

@foster-hangdaan
Copy link
Author

foster-hangdaan commented Aug 21, 2024

But it looks very different than the upstream.

image

image

Thanks for the feedback. More screenshots of the Moon variant will be handy since I based the variant from the single screenshot that was provided on the Neovim port. I also didn't feel like crawling through the Lua code to obtain all the syntax highlighting rules.

Some things that probably won't be fixed based on those screenshots:

  • Language-specific syntax highlighting. Take the Doom port, for example, Elisp's use-package and Rust's let are keywords and are simply following the global rule which specifies that keywords should be magenta. On the other hand we have the Neovim port, where Elisp's use-package is magenta (OK) but then uses a language-specific rule for Rust's let keyword which makes it cyan.

    I believe applying language-specific rules will be a community effort to pull off since it will require testing and modifying font faces for each major mode. It would be helpful if users could submit font face rules for their preferred major modes and I will gladly apply them to this PR.

Some things that will be fixed:

  • Strings are a slightly lighter tint of green.
  • Types should be in blue and not grey. i.e. IpAddr.

@azzamsa
Copy link

azzamsa commented Aug 21, 2024

but then uses a language-specific rule for Rust's let keyword which makes it cyan.

Yes, you are correct, those colors will be changed If I disable the lsp.

Types should be in blue and not grey. i.e. IpAddr.

What about base_url and http.port? is it anguage-specific syntax highlighting.?

@foster-hangdaan
Copy link
Author

foster-hangdaan commented Aug 21, 2024

but then uses a language-specific rule for Rust's let keyword which makes it cyan.

Yes, you are correct, those colors will be changed If I disable the lsp.

Types should be in blue and not grey. i.e. IpAddr.

What about base_url and http.port? is it anguage-specific syntax highlighting.?

I cannot check the Rust major mode since I don't have it nor its LSP installed, but it's probably the same reason why the syntax highlighting are not being applied for TypeScript methods and properties. On the screenshot below, crypto.subtle and data.buffer properties should be in dark-cyan:

image

When I inspect their text properties, they have no associated font-face:

image

Without a font face, there is nothing to target for highlighting, and it seems to fallback to the theme's foreground color.

So now you're probably asking why they don't have an associated font face? Not sure; that is beyond the limits of my Emacs-fu 😢. My blame goes to the major mode and it is somehow not able to identify the syntax. I've looked around and this problem exists in several major modes.

TL;DR: It's an underlying Emacs issue (I think) and is out of the scope of a theme to fix.

@foster-hangdaan
Copy link
Author

foster-hangdaan commented Aug 21, 2024

Also @azzamsa and other Moon variant users, I would like to ask if it's really worth trying to copy the syntax highlighting rules of the Neovim port? Not only will it take a lot of work, but I also think that the Doom highlighting rules are superior since they are more consistent across all major modes, whereas the Neovim port's rules are more arbitrary. Though, I do admit that the Neovim port is more colorful.

@azzamsa
Copy link

azzamsa commented Aug 23, 2024

TL;DR: It's an underlying Emacs issue (I think) and is out of the scope of a theme to fix.

Ah, Okay I see.

Though, I do admit that the Neovim port is more colorful.

It would be nice to look similar to neovim without enabling the lsp (language-specific rule).
We can safely ignore the color from language-specific rules.

@prashantvithani
Copy link

A random suggestion for storm theme: Have type as teal instead of base8.

@prashantvithani
Copy link

A random suggestion for storm theme: Have type as teal instead of base8.

This is to differentiate type from variable as both are set to base8 as of now.

@foster-hangdaan
Copy link
Author

A random suggestion for storm theme: Have type as teal instead of base8.

This is to differentiate type from variable as both are set to base8 as of now.

Sounds good to me. I will also do the same for the light theme and change type to something more colorful than the current dull, grayish color.

This change is for both the Storm and Light variants of Tokyo Night.
i-am-linja added a commit to i-am-linja/themes that referenced this pull request Aug 29, 2024
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.

Add tokyo-night storm and moon
4 participants