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

Don't blind dark theme users (Dark loading screen) #1502

Merged
merged 10 commits into from
Oct 1, 2021

Conversation

peepo5
Copy link
Contributor

@peepo5 peepo5 commented Jul 6, 2021

It is better to start the temporary background dark so it doesnt flashbang dark theme users late at night.
Just happened to me 😢
This makes the loading background color the normal dark theme background color.


Dark Introduction when loading

Pull Request Type
Please select what type of pull request this is:

  • Bugfix
  • Feature Implementation

Related issue
closes #1107

Description
Changes loading background color to dark.

Testing (for code that is not small enough to be easily understandable)
Has this pull request been tested?
Changes Color

It is better to start the temporary background dark so it doesnt flashbang dark theme users late at night.
Just happened to me 😢 
This makes the loading background color the normal dark theme background color.
@efb4f5ff-1298-471a-8973-3d47447115dc

Is this only for dark theme?

Or both dark and night theme?

@peepo5
Copy link
Contributor Author

peepo5 commented Jul 8, 2021

I don't think theme variables can be called from the electron js file. a dark theme is miles better than light for loading in any case. most users use dark so black is not overly important

@dragos-efy
Copy link

dragos-efy commented Jul 8, 2021

I agree with the idea! Very useful! But I think it should be something like hiding the entire body content right before the page loads, then when it does, depending on the dark/light mode it will change dynamically. Here's an example:

Before Page Loads

html,body{background:#555;}
@keyframes pageload { 0% { opacity: 0; } 100% { opacity: 1; } }
body{animation: pageload 0.1s;}

After Page Loads

html,body{background:var(---light-&-dark-background-color-for-this-page-)};

On the current implementation what could be targeted is .dark, .light and .black

@peepo5
Copy link
Contributor Author

peepo5 commented Jul 8, 2021

Dragos, I don't think you understand, no css is used to set loading color, just a hex code in the electron js file inside browserwindow backgroundclor

@dragos-efy
Copy link

Alright, I get you now. I still think that there could be a way to make that dynamic, not sure how tho. That css for setting the color and opacity was from a platform I built in the past, none of it is in freetube. My point was the idea, not the code necessarily. JS can inject css too or change values like yourelement.style.background = 'red'; but there's probably a better way to do it. Someone more skilled than me would know.

@peepo5
Copy link
Contributor Author

peepo5 commented Jul 8, 2021

look at the commit so you get an idea of what changed

@dragos-efy
Copy link

I did lol... before as well... you changed the color, it's one small change

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jul 11, 2021

by the way u have to add the word "closes" before the issue number to link the issue. After this PR is merged the issue will close automatically instead of staying open and clutter the backlog.

Related issue
Before: #1107
After: closes #1107

@dragos-efy
Copy link

I think that here you commented on the wrong one lol. But I added what you said.

@efb4f5ff-1298-471a-8973-3d47447115dc

no i didnt haha. I was being a bot and adding this line to almost every PR that didn't link the issue properly.

@dragos-efy
Copy link

ohhhhh my bad. I forgot it was @peepopoggers who created this pr for a second, so I was confused lmao

@peepo5
Copy link
Contributor Author

peepo5 commented Jul 11, 2021

I do not know this project and electron's capabilities enough to know if this is possible (or needed, effort-wise) but for now this change helps alot. If a way is found then this pr could be a temporary fix. The way to make the background consistent with whatever theme would be reading the css variables into the js file (the commit is in) and then making the background color a variable based on the hex code for the background. As I said, idk how settings & variables are stored, but it might be possible.

@dragos-efy
Copy link

It does, I feel ya. If you wanna target the theme with js, then there's a localstorage variable called baseTheme which can be black, light or dark. And with css (and js as well), the <body> will have a class called either .dark, .black or .light.

I haven't checked the order in which css and js load, cuz it could be that the some things load after and before that line, but when I tested your version a few seconds ago it was opening with the dark background, but then it was light for 1s... that's freetube figuring out what theme is set (cuz light is the default while it recognizes your localstorage variable's value)... and then went dark again, which is normal cuz I was on dark... which means your change works, but only 1/3 times. The other time it doesn't work is if I refresh the app, then it spends 1 second again in light mode figuring out what theme I have. Although to be fair, I don't think that most users refresh the app. So then your change works 1/2 times haha

@PrestonN
Copy link
Member

I think the best solution would be to have the background colour be set based on which base theme is set. index.js should have access to the DB so it should be able to access which theme is set and can change the background colour accordingly. I'd like to see this method implemented instead so that the app starts with whatever theme the user has set.

@peepo5
Copy link
Contributor Author

peepo5 commented Jul 12, 2021

@PrestonN What about the issue mentioned by dragos? Might have to look into that. The way to make the bg color not inconsistent would be to load the color when app is not yet open

@PrestonN
Copy link
Member

Yes, My solution addresses that.

index.js is what runs before the window is loaded, so this is the appropriate location for loading the theme settings and setting the background colour accordingly. This file already accesses the database in a few areas so those could be used as a reference for knowing what theme the user has set and updating the background colour.

@adityab98
Copy link

Thanks for this. Was about to submit one myself. The app always blinds me when I open it haha.

@peepo5
Copy link
Contributor Author

peepo5 commented Jul 20, 2021

I think light being theme loading color needs to change too.

@peepo5
Copy link
Contributor Author

peepo5 commented Sep 23, 2021

The reason this happens is due to two issues:

  • Electron index.js BrowserWindow background color being white.
  • Light theme as placeholder when theme is not loaded in yet.

@PrestonN PrestonN enabled auto-merge (squash) September 23, 2021 22:06
@peepo5
Copy link
Contributor Author

peepo5 commented Sep 23, 2021

Updated and there is no white flash for me anymore (except for some weird cases). I tried to use localstorage in index.js but FreeTube would not launch without showing any errors, so I used the default dark mode for the beginning launch. The main change is moving the checkThemeSettings() to before the other functions so that it is not delayed. I would appreciate testers.

Edit: I also think that there may be a tiny white flash the first time you use the app, but nearly every other time it does not flash. It is weird because I think if there is some interruption while starting, there is a possibility of a flash. Is created() the earliest function available to check for themes? It's annoying because it is not consistent enough for this to be ready to be merged :<

@peepo5
Copy link
Contributor Author

peepo5 commented Sep 29, 2021

I will document the flashing difference with dev and without dev if any later today

@peepo5
Copy link
Contributor Author

peepo5 commented Sep 29, 2021

With that change it looks consistent when built but it needs testing

@PikachuEXE
Copy link
Collaborator

With this change the app flash issue has become at least less noticeable.
I would like see this to be included in the next release if possible.

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Sep 29, 2021

@peepopoggers
Hey I just spotted that
https://github.com/peepopoggers/FreeTube/blob/patch-1/src/index.ejs#L19
still has light mainRed secBlue instead of dark mainRed secBlue like my patch suggested
Is this intended?

Edit:
I still see the flash with light mainRed secBlue but I don't see it with dark mainRed secBlue

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

One change left for https://github.com/FreeTubeApp/FreeTube/blob/development/src/index.ejs#L19 to fix the flash

light mainRed secBlue

incomplete.mov

dark mainRed secBlue

complete.mov

@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 Sep 29, 2021
@peepo5
Copy link
Contributor Author

peepo5 commented Sep 29, 2021

@PikachuEXE I did not see that change! My mistake. Will change promptly.

@peepo5 peepo5 added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Sep 29, 2021
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested via npm run debug

Screen.Recording.2021-09-30.at.10.20.45.AM.mov

@PikachuEXE
Copy link
Collaborator

1 more to go!

@peepo5 peepo5 requested review from Svallinn and Hiers October 1, 2021 08:30
@peepo5
Copy link
Contributor Author

peepo5 commented Oct 1, 2021

@efb4f5ff-1298-471a-8973-3d47447115dc Could you test this :)

@efb4f5ff-1298-471a-8973-3d47447115dc

already did waiting for approval to merge this

@PikachuEXE
Copy link
Collaborator

@PrestonN PrestonN merged commit 25189e2 into FreeTubeApp:development Oct 1, 2021
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 10, 2022
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.

Dark background when starting the app
7 participants