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.

Sign up for GitHub

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

Jump to bottom

update catppucin themes #4581

Closed
wants to merge 5 commits into from
Closed

update catppucin themes #4581

wants to merge 5 commits into from

Conversation

ivktac
Copy link
Contributor

@ivktac ivktac commented Nov 3, 2022

  • added missing scopes attribute, constant.builtin, punctuation.special, ui.cursorcolumn.primary, diagnostic.*
  • changed foreground text in statusline and bufferline to make more visible
  • changed dark theme to the extended version
  • patched scopes to another colors for better highlights

ChrHorn
ChrHorn reviewed Nov 3, 2022
View reviewed changes
Copy link
Contributor

@ChrHorn ChrHorn left a comment

Choose a reason for hiding this comment

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

Left some comments for the most obvious things that I noticed. Note that the helix catppuccin theme is basically a 1 to 1 copy of the neovim theme and this deviates quite a lot from it.

runtime/themes/catppuccin_latte.toml Outdated

"constructor" = "sapphire"

"constant" = "peach"
"constant.builtin" = "peach"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant if constant with the same color is already defined

runtime/themes/catppuccin_latte.toml Outdated

"comment" = { fg = "overlay1", modifiers = ["italic"] }
"comment" = { fg = "surface2", modifiers = ["italic"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

This used to be surface2 but it was changed to improve visibility catppuccin/helix#6

runtime/themes/catppuccin_latte.toml Outdated
@@ -31,9 +32,10 @@

"function" = "blue"
"function.builtin" = "peach"
"function.macro" = "teal"
"function.macro" = "mauve"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of this change. This makes macros the same colors as keywords.

runtime/themes/catppuccin_latte.toml Outdated
"ui.bufferline.active" = { fg = "text", bg = "surface0", modifiers = ["bold"] }
"ui.bufferline.background" = { bg = "surface1" }
"ui.bufferline" = { fg = "surface1", bg = "mantle" }
"ui.bufferline.active" = { fg = "text", bg = "base", modifiers = ["bold", "italic"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need italic here, only bold looks better

runtime/themes/catppuccin_latte.toml Outdated
Comment on lines 106 to 109
"diagnostic.error" = "red"
"diagnostic.warn" = "yellow"
"diagnostic.info" = "sky"
"diagnostic.hint" = "teal"
Copy link
Contributor

@ChrHorn ChrHorn Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

These should be redundant. Diagnostics already use the error, warning, ... colors.

runtime/themes/catppuccin_frappe.toml Outdated
# User Interface
# --------------
"ui.background" = { fg = "text", bg = "base" }
inherits = "catppuccin_latte"

"ui.linenr" = { fg = "surface1" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. The main idea was to make easy update themes for new features in the future

runtime/themes/catppuccin_frappe.toml Outdated
warning = "yellow"
info = "sky"
hint = "teal"
"ui.cursorcolumn.primary" = { bg = "cursorline" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

runtime/themes/catppuccin_frappe.toml Outdated Show resolved Hide resolved
runtime/themes/catppuccin_frappe.toml Outdated Show resolved Hide resolved
runtime/themes/catppuccin_latte.toml Outdated

"ui.highlight" = { bg = "surface1" }
"ui.highlight" = { bg = "surface1", modifiers = ["bold"] }
Copy link
Contributor

@ChrHorn ChrHorn Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why the change here? This is used to highlight code blocks in the symbol picker.

ivktac and others added 3 commits November 3, 2022 14:27
@ivktac @ChrHorn
fix spelling
f892f25 
Co-authored-by: ChrHorn <christoph-horn@hotmail.de>
@ivktac @ChrHorn
fix color hex to correct
0158df1 
Co-authored-by: ChrHorn <christoph-horn@hotmail.de>
@ivktac
update theme
faf3aa3
@ivktac
Copy link
Contributor Author

ivktac commented Nov 3, 2022 •
edited
Loading

I applied your suggestions. Could you @ChrHorn review it again?

@kirawi kirawi added A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 4, 2022
@ChrHorn
Copy link
Contributor

ChrHorn commented Nov 6, 2022

Ok, I managed to take a look at it.

Personally, I am against the changes to the UI. The theme is a carbon copy of them neovim variant and I would prefer to keep it that way. The lower contrast ratio on some UI elements is intentional and only on elements that should not draw too much focus.

I am fine with:

  • Usage of inheritance, but could you use mocha as the base theme? It is basically the base cappuccino theme. Light and dark variants should be able to inherit just fine without any extra modifications needed, just use the different palettes.
  • The additional scopes and the cursorcolumn addition
  • The color changes for links and diffs

Also, could you restore "type.enum.variant" = "peach". It's not in the docs but is used by some queries.

@kirawi kirawi added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 7, 2022
@archseer
Copy link
Member

Are all the remaining items resolved here now?

@ChrHorn
Copy link
Contributor

ChrHorn commented Nov 18, 2022

Not yet, needs some more adjustments.

@ivktac ivktac closed this Jan 6, 2023
@ivktac
Copy link
Contributor Author

ivktac commented Jan 6, 2023

superseded by #5404

@ivktac ivktac deleted the update-catppuccin-themes branch January 6, 2023 10:04
@archseer archseer mentioned this pull request Jan 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrHorn ChrHorn ChrHorn left review comments

Assignees
No one assigned
Labels
A-theme Area: Theme and appearence related S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivktac @ChrHorn @archseer @kirawi