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

Add rstheme_terminal_colors #63

Merged
merged 22 commits into from
Nov 12, 2021
Merged

Conversation

nsgrantham
Copy link
Contributor

Addresses #61. I implemented your recommended changes, the rainbow parens was a helpful example to follow, thank you. I tested this on my end and it's looking good. Let me know if there is something you'd like me to take a second pass at!

@nsgrantham
Copy link
Contributor Author

Before:
Screen Shot 2021-03-30 at 11 39 58 PM

After:
Screen Shot 2021-03-30 at 11 38 35 PM

Here's the code where I use rstheme_terminal_colors: https://github.com/nsgrantham/poolside-rstudio/blob/main/poolside.R

Copy link
Owner

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

This is great, thank you @nsgrantham! I also really like your poolside theme and I'd be happy for it to be part of rsthemes if that's something you're interested in.

I made one small suggestion to include your handle in the NEWS item.

I also need to check that the _terminal.scss partial isn't included twice when using rstheme_terminal_colors(). That might not be trivial to change; I think it's included in the base themes directly since it hasn't yet been customizable.

NEWS.md Outdated Show resolved Hide resolved
Co-authored-by: Garrick Aden-Buie <garrick@adenbuie.com>
@nsgrantham
Copy link
Contributor Author

Thanks, added my username to the NEWS item.

Glad you like Poolside! I'd love to add it to rsthemes, I'll open a PR when I'm done fiddling with it.

I don't entirely understand the _terminal.scss issue — does this mean the base themes need to be modified to use rstheme_terminal_colors() otherwise the partial is included twice?

@gadenbuie
Copy link
Owner

Thanks @nsgrantham and don't worry, I'll handle the _terminal.scss issue

@gadenbuie
Copy link
Owner

@nsgrantham sorry for the delay on this and thanks again for contributing the PR!

I tweaked the function signature and default arguments a bit. rstheme_terminal_colors() now picks up the theme's foreground and background colors and uses them for white and black. I also configured the args so that the _bright colors inherit from their non-bright colors, which makes it easier when you don't have a bright shade.

I also went through and added terminal colors to most themes in rsthemes, which is exciting!

@gadenbuie gadenbuie merged commit 4e44057 into gadenbuie:main Nov 12, 2021
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.

2 participants