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

Rosé Pine theme: use official variants #5489

Merged
merged 5 commits into from
Jan 12, 2023
Merged

Rosé Pine theme: use official variants #5489

merged 5 commits into from
Jan 12, 2023

Conversation

mvllow
Copy link
Contributor

@mvllow mvllow commented Jan 10, 2023

Thanks to the lovely community, we noticed our themes were included in Helix very early on. This PR updates the variants to match our current spec and palette.

We hope contributors make a PR at https://github.com/rose-pine/helix for any theme changes but understand this may not always be the case. For that reason, our Moon and Dawn variants inherit from the Main variant to try and keep them in sync as much as possible.

Please let me know if anything else is needed, and thank you 💜

@mvllow mvllow marked this pull request as draft January 10, 2023 21:37
@the-mikedavis the-mikedavis added A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 10, 2023
@mvllow
Copy link
Contributor Author

mvllow commented Jan 10, 2023

After running cargo xtask themelint rose_pine I got the following log:

rose_pine: `ui.selection` and `ui.selection.primary` cannot be equal
rose_pine: missing `fg` for `ui.virtual.whitespace`
rose_pine: missing `fg` and `bg` for `ui.help`
rose_pine: missing `fg` or `bg` for `ui.window`

Will update these asap :)

@mvllow
Copy link
Contributor Author

mvllow commented Jan 10, 2023

Lint errors have been fixed for rose_pine but running it on rose_pine_moon or rose_pine_dawn causes several errors due to these variants not explicitly setting any theme values but rather inheriting from rose_pine. Is this okay or should I go ahead and make each variant independent from one another?

@the-mikedavis
Copy link
Member

For those you can ignore the errors: I would prefer that they use inherits rather than pass the themelint

@mvllow mvllow marked this pull request as ready for review January 10, 2023 23:27
@archseer
Copy link
Member

Sorry, slight conflict now since a previous PR changed the diagnostics to use curly underlines.

Thank you for maintaining these now officially upstream!

@cor
Copy link
Contributor

cor commented Jan 12, 2023

I like how this PR will ensure that the new curl underlines that I applied to only the normal variant will also work for the other variants 👍🏻

@the-mikedavis the-mikedavis merged commit 051cd78 into helix-editor:master Jan 12, 2023
@mvllow mvllow deleted the update-rose-pine-theme branch January 12, 2023 23:29
kirawi pushed a commit to kirawi/helix that referenced this pull request Jan 25, 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.

None yet

4 participants