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 Solarized dark and Solarized light color scheme #5066

Merged
merged 12 commits into from
May 8, 2024

Conversation

DontBlameMe99
Copy link
Contributor

@DontBlameMe99 DontBlameMe99 commented May 5, 2024

Add Solarized dark and Solarized light color scheme

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Description

I implemented the solarized dark and light color scheme, its accent colors and corresponding icons.
https://github.com/altercation/solarized

Things I added:

  • Base Theme Solarized Dark
  • Base Theme Solarized Light
  • Main/Secondary Color Theme Solarized Yellow
  • Main/Secondary Color Theme Solarized Orange
  • Main/Secondary Color Theme Solarized Red
  • Main/Secondary Color Theme Solarized Magenta
  • Main/Secondary Color Theme Solarized Violet
  • Main/Secondary Color Theme Solarized Blue
  • Main/Secondary Color Theme Solarized Cyan
  • Main/Secondary Color Theme Solarized Green

Screenshots

image
image

Desktop

  • OS: Linux
  • FreeTube version: Latest development branch

Add the solarized dark and the solarized light color scheme, as well as accent colors and icons
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 5, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 5, 2024 12:33
@DontBlameMe99 DontBlameMe99 changed the title feat: ✨ add Solarized dark and Solarized light color scheme Add Solarized dark and Solarized light color scheme May 5, 2024
Made colors, especially in light mode, more readable.
auto-merge was automatically disabled May 5, 2024 12:59

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 5, 2024 12:59
DontBlameMe added 2 commits May 5, 2024 15:06
Add a missing Solarized-Light branch to the check
Remove a useless empty line
auto-merge was automatically disabled May 5, 2024 13:06

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 5, 2024 13:06
@kommunarr
Copy link
Collaborator

Thanks for the PR! You'll also need to modify the ft-share-button SCSS (note to team: we should honestly centralize this in themes.css in the future as this is very easy to miss). What this governs is what version of the Invidious and YouTube logos show when you click the "Share" icon under a video. If the logo is too dark / bright to see with a given base theme, it means it needs to put it in with either the dark or light colors in that file.

Copy link
Member

Choose a reason for hiding this comment

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

Everything LGTM, im a bit confused why dark and light theme have the same return value
https://github.com/FreeTubeApp/FreeTube/pull/5066/files#diff-f4ca67281c6be819c2166841d7631ffd5316d4c3a3b40e01a512c4113a537b2cR630

Will approve after @jasonhenriquez signs off

@DontBlameMe99
Copy link
Contributor Author

Everything LGTM, im a bit confused why dark and light theme have the same return value https://github.com/FreeTubeApp/FreeTube/pull/5066/files#diff-f4ca67281c6be819c2166841d7631ffd5316d4c3a3b40e01a512c4113a537b2cR630

Will approve after @jasonhenriquez signs off

Honestly because I don't really understand what they mean (in terms of what they are used for), and therefore I don't know what the light/dark one should be

@kommunarr
Copy link
Collaborator

Honestly because I don't really understand what they mean (in terms of what they are used for), and therefore I don't know what the light/dark one should be

This is the startup color while the app is loading. You should have it match the main bg-color for that given theme.

@DontBlameMe99
Copy link
Contributor Author

Honestly because I don't really understand what they mean (in terms of what they are used for), and therefore I don't know what the light/dark one should be

This is the startup color while the app is loading. You should have it match the main bg-color for that given theme.

Thanks, I'll change it!

auto-merge was automatically disabled May 5, 2024 13:47

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 5, 2024 13:47
@DontBlameMe99
Copy link
Contributor Author

DontBlameMe99 commented May 5, 2024

Both things have been changed, I hope this is what you meant :) @jasonhenriquez

src/renderer/themes.css Outdated Show resolved Hide resolved
@kommunarr kommunarr mentioned this pull request May 6, 2024
1 task
kommunarr
kommunarr previously approved these changes May 6, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 7, 2024 15:29
auto-merge was automatically disabled May 7, 2024 15:36

Head branch was pushed to by a user without write access

@kommunarr
Copy link
Collaborator

Hi @DontBlameMe99, could you group the primary and secondary solarized theme colors by light and dark respectively? Dracula code for reference:

Primary colors

.mainDraculaCyan,
.mainDraculaGreen,
.mainDraculaOrange,
.mainDraculaRed,
.mainDraculaYellow {
--text-with-main-color: #282A36;
--logo-icon-bar-color: url("../../_icons/iconDraculaDarkSmall.svg");
--logo-text-bar-color: url("../../_icons/textDraculaDarkSmall.svg");
}
.mainDraculaPink,
.mainDraculaPurple {
--text-with-main-color: #F8F8F2;
--logo-icon-bar-color: url("../../_icons/iconDraculaLightSmall.svg");
--logo-text-bar-color: url("../../_icons/textDraculaLightSmall.svg");
}

Secondary colors

.secDraculaCyan,
.secDraculaGreen,
.secDraculaOrange,
.secDraculaRed,
.secDraculaYellow {
--text-with-accent-color: #212121;
}
.secDraculaPink,
.secDraculaPurple {
--text-with-accent-color: #F8F8F2;
}

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 7, 2024 15:36
auto-merge was automatically disabled May 7, 2024 15:44

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 7, 2024 15:44
@DontBlameMe99
Copy link
Contributor Author

Hi @DontBlameMe99, could you group the primary and secondary solarized theme colors by light and dark respectively? Dracula code for reference:

Primary colors

.mainDraculaCyan,
.mainDraculaGreen,
.mainDraculaOrange,
.mainDraculaRed,
.mainDraculaYellow {
--text-with-main-color: #282A36;
--logo-icon-bar-color: url("../../_icons/iconDraculaDarkSmall.svg");
--logo-text-bar-color: url("../../_icons/textDraculaDarkSmall.svg");
}
.mainDraculaPink,
.mainDraculaPurple {
--text-with-main-color: #F8F8F2;
--logo-icon-bar-color: url("../../_icons/iconDraculaLightSmall.svg");
--logo-text-bar-color: url("../../_icons/textDraculaLightSmall.svg");
}

Secondary colors

.secDraculaCyan,
.secDraculaGreen,
.secDraculaOrange,
.secDraculaRed,
.secDraculaYellow {
--text-with-accent-color: #212121;
}
.secDraculaPink,
.secDraculaPurple {
--text-with-accent-color: #F8F8F2;
}

implemented what you requested. I think the color contrast should be fine now, I checked everything on the provided website.

auto-merge was automatically disabled May 7, 2024 16:39

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 7, 2024 16:39
kommunarr
kommunarr previously approved these changes May 7, 2024
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

Thanks for the quick responses to feedback, I know theme additions can be a hassle. LGTM!

src/renderer/themes.css Outdated Show resolved Hide resolved
auto-merge was automatically disabled May 8, 2024 00:38

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 8, 2024 00:38
Copy link
Member

Choose a reason for hiding this comment

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

Thankyou for putting this together @DontBlameMe99 !

@FreeTubeBot FreeTubeBot merged commit 0ca803e into FreeTubeApp:development May 8, 2024
10 checks passed
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.

None yet

5 participants