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

feat: add whiskers palette file and documentation #3

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

dacid44
Copy link
Contributor

@dacid44 dacid44 commented Jul 17, 2024

resolves #2

@dacid44
Copy link
Contributor Author

dacid44 commented Jul 17, 2024

If you accept this, I'm not sure how you'd want it linked into the main README.md.

@ozwaldorf ozwaldorf force-pushed the main branch 9 times, most recently from 5be8870 to f11a2c9 Compare July 17, 2024 04:40
@ozwaldorf
Copy link
Owner

Whoops, what a coincidence, sorry for the force pushes!

This is a great addition and opens the door to a lot of easy access to ports. Honestly, I wouldn't mind just having whiskers.json at the top level and documentation along with the other usage sections

@ozwaldorf
Copy link
Owner

ozwaldorf commented Jul 17, 2024

I wonder if this could also be integrated into the nix flake, through hm modules (ie, if zed reads themes from a dir, we can generate and write it there). Not a requirement just ideating

@dacid44
Copy link
Contributor Author

dacid44 commented Jul 17, 2024

I've never worked with Nix before, but tomorrow I can move this stuff into the top level and try to resolve the force push conflicts at least.

@dacid44
Copy link
Contributor Author

dacid44 commented Jul 17, 2024

By the way, this is what the resulting Zed color scheme looks like:
image

I tested with the whiskers CLI tool a bit more and found which exact
colors cause it to break. I changed the rest of the colors back to match
the rest of the Carburetor theme.
@@ -0,0 +1,86 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the nit, but could probably have this in examples/whiskers.json

Otherwise lgtm!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, I'd argue you might want to rename the examples directory in that case though. The stuff in there right now is less "examples" and more "here, it's done for you already". Not that it matters too much as long as the documentation links to it properly.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree, it was previously src, I might move it back to that

README.md Show resolved Hide resolved
Copy link
Owner

@ozwaldorf ozwaldorf left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the work, once whiskers is updated upstream, would you mind making a follow up pr to amend the breaking colors?

@ozwaldorf ozwaldorf merged commit 2dc56e8 into ozwaldorf:main Jul 18, 2024
5 checks passed
@dacid44
Copy link
Contributor Author

dacid44 commented Jul 19, 2024

Sure, if I don't lose track of it. Here's the issue from whiskers: catppuccin/whiskers#32

Though for now, it's probably fine. Only about 3 colors needed to be changed in the end, and not by enough that it would really be noticeable. Still, it does annoy me to have to change the colors just to work around a bug.

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 support for catppuccin-whiskers
2 participants