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

Create "light mode" theme #2959

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Create "light mode" theme #2959

wants to merge 29 commits into from

Conversation

jls47
Copy link
Contributor

@jls47 jls47 commented Oct 13, 2023

Hopefully resolves #164 . Implements a number of changes in color scheme and adds a couple new logo .svgs for use in the top left and login screen of the app. Can be toggled by the "light mode" (now changed from "bright mode") checkbox that's now in the preferences menu.

The reason this says -hopefully- is because part of the changes includes changing the user agent used to render the webview (in /src/renderer/pages/BrowserPage/index.tsx). Rather than {useragent.userAgent()}, I changed it to that string returned by the function but removed all mention of kitch and itch when the bright mode checkbox is selected. While this prevents the need for JS injection and consequent theme flashing, it does seem to affect what shows up on the home "explore" screen as a result. I'd love to explore some ways to get this working if possible.

Screenshot from 2023-10-13 14-26-56
Screenshot from 2023-10-13 14-27-02

@jls47
Copy link
Contributor Author

jls47 commented Oct 15, 2023

If I may propose a solution-could we set it up so that adding something like "itch-light kitch-light" to the user agent would produce the same functionality minus the forced dark_theme class getting added to the body?

For what it's worth, this does not appear to affect whether or not games can be installed, just whether games are filtered to the user's OS by default and whether "install app" shows up on the home page.

Screenshot from 2023-10-16 14-07-51

Copy link

@3ter 3ter left a comment

Choose a reason for hiding this comment

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

I couldn't test the version because I don't seem to get after the captcha when logging into kitch. But this is the same for the master branch in the upstream repo.

I don't understand why this hasn't been yet integrated into the main repo. Or at least been commented on. It seems rather complete, with translations and all.

Only one file still contains some commented out code raising questions 😃.

@@ -14,6 +14,12 @@ const PreferencesDiv = styled.div`
${styles.meat};
`;

const expTextColor = global.ReduxStore.getState().preferences.lightMode
Copy link

Choose a reason for hiding this comment

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

What does exp stand for, "expression"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If memory serves, this is attached to explanatory text in the preferences menu. I'll have to get to the other comments and suggestions as the week goes, just happy to hear back about this!

src="about:blank"
ref={this.gotWebview}
partition={partition}
useragent="Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/108.0.5359.215 Electron/22.3.14 Safari/537.36"
Copy link

Choose a reason for hiding this comment

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

Why is there a hardcoded version number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. This is a somewhat hacky solution I landed on and didn't readdress-I'll see about using some string methods to remove instances of itch/kitch instead while not having to use something as gross as hardcoding.

Copy link
Contributor Author

@jls47 jls47 Mar 6, 2024

Choose a reason for hiding this comment

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

@3ter After doing some experimenting, I'm struggling to find a way that isn't annoying to address this programmatically so I'll just remove the version numbers for now. If you have any tips on accomplishing this via other means I am very open to them!

if (sleepy && !visible) {
return null;
}

//Changes based on the bright mode checkbox
Copy link

Choose a reason for hiding this comment

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

Shoul be called light instead of bright for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. Does keeping the "bright" nomenclature for user-facing purposes make sense though? I found it to be probably the most reliably translatable way to communicate the functionality across the different languages that itch majorly supports.

Copy link

Choose a reason for hiding this comment

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

Light mode seems much more common as a term for this feature (e.g. Google search for app light/bright mode 😅 ). I can only speak for English though.

@@ -20,11 +20,21 @@ export const FilterGroup = styled.div`

// const inactiveBg = `linear-gradient(to top,hsla(355, 43%, 25%, 1),hsla(355, 43%, 17%, 1))`;
// const activeBg = `linear-gradient(to top, hsla(355, 43%, 50%, 1), hsla(355, 43%, 37%, 1));`;
const inactiveBg = `linear-gradient(to top,hsla(355, 43%, 17%, 1),hsla(355, 43%, 11%, 1))`;
const inactiveBg = `linear-gradient(to top, hsla(355, 48%, 38%, 1), hsla(355, 48%, 27%, 1));`;
Copy link

Choose a reason for hiding this comment

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

It is unclear if these changes are intended for both dark and light mode. The comment below should be deleted or replace the current definition.

Copy link

Choose a reason for hiding this comment

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

It's clear now.

const borderColor = `#843442`;
const borderRadius = `4px`;

//color: ${(props) => props.theme.baseText};
Copy link

Choose a reason for hiding this comment

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

Same as above.

@jls47 jls47 changed the title Create "bright mode" theme Create "light mode" theme Mar 7, 2024
Copy link

@3ter 3ter left a comment

Choose a reason for hiding this comment

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

I like the way the switch in the settings is working.

One observation I made was this:
image

image

It would be cool for them to be in sync, but the PR is already big enough for my taste.

I unfortunately don't have time to look as deep into it as I would like to but what I saw is of good quality! I really appreciate your contribution!

@@ -20,11 +20,21 @@ export const FilterGroup = styled.div`

// const inactiveBg = `linear-gradient(to top,hsla(355, 43%, 25%, 1),hsla(355, 43%, 17%, 1))`;
// const activeBg = `linear-gradient(to top, hsla(355, 43%, 50%, 1), hsla(355, 43%, 37%, 1));`;
const inactiveBg = `linear-gradient(to top,hsla(355, 43%, 17%, 1),hsla(355, 43%, 11%, 1))`;
const inactiveBg = `linear-gradient(to top, hsla(355, 48%, 38%, 1), hsla(355, 48%, 27%, 1));`;
Copy link

Choose a reason for hiding this comment

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

It's clear now.

@@ -35,7 +34,7 @@ const optionButtonLike = css`
margin: 0;
border: 1px solid ${borderColor};
border-left: none;
color: ${(props) => props.theme.baseText};
color: #fffff0;
Copy link

Choose a reason for hiding this comment

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

Why is this not depending on the theme anymore? It's more an observation, but I found that e.g. in src\renderer\modal-widgets\PlanInstall\index.tsx there's still a

color: ${(props) => props.theme.baseText};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, don't know that this was intended. I've reverted, thank you for pointing this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@3ter Pushed the final bit into the pull request-had missed the "game management" modal window. Should be no more changes from here, unless something's off that I didn't see.

@jls47
Copy link
Contributor Author

jls47 commented Oct 3, 2024

Is anyone on macos able to see why this is failing?

@jls47
Copy link
Contributor Author

jls47 commented Oct 14, 2024

There we are, merged the latest changes from master and everything seems to be up to snuff. Let me know if anything else is needed here. Should also address #2852.

@jls47 jls47 mentioned this pull request Oct 26, 2024
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 a light theme
2 participants