-
Notifications
You must be signed in to change notification settings - Fork 888
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 gruvbox base theme and gruvbox green color theme #4805
Conversation
Ah, lint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening your first pull request on the FreeTube repository 🥳.
Please fix the linter errors, if you switch to the files changed tab, you can see all the alerts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stuff disappears when both primary and secondary colors are set to gruvbox green
VirtualBoxVM_Lt2oQJ2LzF.mp4
Also this doesnt fully close the issue because you didnt implement all the gruvbox colors listed in their repo https://github.com/morhetz/gruvbox. The issue also explicitly asks for the light and dark theme implementation. Would you be open to implementing all the other colors and both themes? |
Yes, I will implement both light and dark, I was just checking if this pr is appropriate. I'll fix the secondary colour issue right now, |
Head branch was pushed to by a user without write access
… add the opacity1 - 4
Head branch was pushed to by a user without write access
Ok, so the reason the secondary colours were not working properaly was because I did not add the "--accent-color-opacity1 - 4" so I added that and I think it's fixed though I haven't added the gruv light theme yet. |
By the way, I'm not sure how to make the icons for it so mine were a bit thrown together and look not great, so yeah. |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
…uvbox (and fixed lint)
Head branch was pushed to by a user without write access
@@ -31,6 +31,13 @@ export const colors = [ | |||
{ name: 'CatppuccinMochaSapphire', value: '#74C7EC' }, | |||
{ name: 'CatppuccinMochaBlue', value: '#89B4FA' }, | |||
{ name: 'CatppuccinMochaLavender', value: '#B4BEFE' }, | |||
{ name: 'GruvboxGreen', value: '#b8bb26' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this below the dracula ones
Head branch was pushed to by a user without write access
src/main/index.js
Outdated
@@ -626,6 +626,8 @@ function runApp() { | |||
return '#de1c85' | |||
case 'nordic': | |||
return '#2b2f3a' | |||
case 'gruvbox-dark': | |||
return '282828' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated to the new bg-color of 32302f
.
src/renderer/themes.css
Outdated
.mainGruvboxPurple, | ||
.mainGruvboxAqua, | ||
.mainGruvboxOrange { | ||
--text-with-main-color: #282828; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text-with-main-color
, and the other --text-with-accent-color
, (which should also both be the same color) are still at a notably low color contrast with mainGruvboxRed
and secGruvboxRed
(4.29:1 and 4.77:1 respectively). Ideally this should be a 7:1 contrast to meet the WCAG AAA standard.
Co-authored-by: Jason <[email protected]>
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Remove extraneous reference to gruvBoxLight Co-authored-by: Jason <[email protected]>
… WCAG AAA standard.
Head branch was pushed to by a user without write access
@DeaDvey some code reviews are still unresolved/not implemented |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
This PR was closed because it has been stalled for 14 days with no activity. |
Add gruvbox base theme and gruvbox green color theme
Pull Request Type
Related issue
Closes #3987
Description
I added a gruvbox base theme using the gruvbox colour palette as well as a gruvbox green color theme, I will add more of the color themes if the pr is accepted
Screenshots
Testing
I tested it and I cannot see any issues with the color schemes showing though I have not added all the translations as I only speak english.
Desktop
Additional context
I wanted a gruvbox theme for FreeTube and I saw that another issue request did too, it's fine if you (the developers) don't, I was just opening this pr incase it was appropriate.