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 gruvbox base theme and gruvbox green color theme #4805

Closed
wants to merge 34 commits into from

Conversation

DeaDvey
Copy link

@DeaDvey DeaDvey commented Mar 25, 2024

Add gruvbox base theme and gruvbox green color theme

Pull Request Type

  • Feature Implementation

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

settings page
subscriptions page
watch page

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

  • OS: GNU/Linux
  • OS Version: openSUSE Tumbleweed
  • FreeTube version: most up to date one on github

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.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 25, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 25, 2024 18:01
@DeaDvey
Copy link
Author

DeaDvey commented Mar 25, 2024

Ah, lint.

Copy link
Member

@absidue absidue left a 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.

Copy link
Member

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

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Mar 25, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Mar 25, 2024

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?

@DeaDvey
Copy link
Author

DeaDvey commented Mar 25, 2024

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,
Thanks

auto-merge was automatically disabled March 25, 2024 22:10

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 25, 2024 22:10
auto-merge was automatically disabled March 25, 2024 22:26

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 25, 2024 22:26
@DeaDvey
Copy link
Author

DeaDvey commented Mar 25, 2024

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.

@DeaDvey
Copy link
Author

DeaDvey commented Mar 25, 2024

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.

auto-merge was automatically disabled March 25, 2024 23:01

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 25, 2024 23:02
auto-merge was automatically disabled March 25, 2024 23:03

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 25, 2024 23:03
auto-merge was automatically disabled March 26, 2024 00:19

Head branch was pushed to by a user without write access

removed unnecessary extra blank line

Co-authored-by: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 1, 2024 22:49
@@ -31,6 +31,13 @@ export const colors = [
{ name: 'CatppuccinMochaSapphire', value: '#74C7EC' },
{ name: 'CatppuccinMochaBlue', value: '#89B4FA' },
{ name: 'CatppuccinMochaLavender', value: '#B4BEFE' },
{ name: 'GruvboxGreen', value: '#b8bb26' },

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

auto-merge was automatically disabled June 1, 2024 23:09

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 1, 2024 23:09
@@ -626,6 +626,8 @@ function runApp() {
return '#de1c85'
case 'nordic':
return '#2b2f3a'
case 'gruvbox-dark':
return '282828'
Copy link
Collaborator

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.

.mainGruvboxPurple,
.mainGruvboxAqua,
.mainGruvboxOrange {
--text-with-main-color: #282828;
Copy link
Collaborator

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 <84899178+jasonhenriquez@users.noreply.github.com>
auto-merge was automatically disabled June 6, 2024 11:57

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 6, 2024 11:57
auto-merge was automatically disabled June 6, 2024 11:57

Head branch was pushed to by a user without write access

Remove extraneous reference to gruvBoxLight

Co-authored-by: Jason <84899178+jasonhenriquez@users.noreply.github.com>
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 6, 2024 11:58
auto-merge was automatically disabled June 6, 2024 12:13

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 6, 2024 12:13
@efb4f5ff-1298-471a-8973-3d47447115dc

@DeaDvey some code reviews are still unresolved/not implemented

Copy link
Contributor

github-actions bot commented Jul 5, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Aug 3, 2024

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.

Copy link
Contributor

This PR was closed because it has been stalled for 14 days with no activity.

@github-actions github-actions bot closed this Aug 28, 2024
auto-merge was automatically disabled August 28, 2024 01:53

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add Gruvbox themes.
6 participants