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

Update styles.css Shorts Background #2322

Merged
merged 2 commits into from
May 31, 2024
Merged

Conversation

raszpl
Copy link
Contributor

@raszpl raszpl commented May 31, 2024

@ImprovedTube ImprovedTube merged commit e653ff6 into code-charity:master May 31, 2024
@ImprovedTube
Copy link
Member

yay!!

@ImprovedTube
Copy link
Member

does removing these matter?

	case 'light':
		document.documentElement.removeAttribute('dark');
		document.querySelector('ytd-masthead')?.removeAttribute('dark');

@raszpl raszpl deleted the patch-3 branch May 31, 2024 20:32
@raszpl
Copy link
Contributor Author

raszpl commented May 31, 2024

Removing dark shouldnt matter for the most part, there are still some missing CSS rules tho like loading placeholders for icons/thumbnails - those show up as black rectangles with dark cookie. Im too lazy to check what to override there.

I moved it down because if 'light' gets it there is no reason for all other themes not to get it. Still this would only matter if ImprovedTube.setTheme() was actually called during loading, currently its not. Im it should be merged with myColors() again.

@ImprovedTube
Copy link
Member

ImprovedTube commented May 31, 2024

then you didn't mean to remove them at case 'light' ?
Didnt find a reason for the function to run always.

when switching themes ambient mode from 'dark' might stay with 'light' or custom colors, which was fixed 1 year ago, so i'm fetching another line from the old file.

@ImprovedTube
Copy link
Member

then you didn't mean to remove them at case 'light' ?
Didn't find a reason for the function to run always.

did you see/get, when switching themes ambient mode from 'dark' might stay with 'light' or custom colors (was fixed 1 year ago, so i'm fetching another line from the old file)

@raszpl
Copy link
Contributor Author

raszpl commented Jun 2, 2024

then you didn't mean to remove them at case 'light' ?

nothing was removed, '// fall through' comment is there to remind people of that
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch#breaking_and_fall-through
Those line breaks inside Switch statement are there for a reason.

Didn't find a reason for the function to run always.

the reason is:

did you see/get, when switching themes ambient mode from 'dark' might stay with 'light' or custom colors (was fixed 1 year ago, so i'm fetching another line from the old file)

thats why you need to run myThemes(),. and since you need to run both themes and colors there is no point not combining them into one
I dont get why you feel hiding cinematics makes sense for some themes when we have explicit option for 'Ambient lighting'. No matter, which ones should it be disabled for? all except dark and black?

40b586e

if(document.getElementById("cinematics") !== null)

#2042 (comment)

but you dont even need explicit comparison block there to begin with.

document.getElementById("cinematics").style.display = 'none !important';}

never worked, cant set !important thru .style.display interface, only accepts a specific fixed list of parameters https://www.w3schools.com/jsref/prop_style_display.asp

to actually make !important stick you need either

document.getElementById("cinematics").setAttribute( 'style', 'display: none !important' );
document.getElementById("cinematics").style.setProperty("display", "none", "important");

Testing now you dont need !important there at all.

{document.getElementById("cinematics").style.visibility = 'hidden';
document.getElementById("cinematics").style.display = 'none !important';}

do you want display none or invisible? display:none and visibility:hidden are two distinctly different things

#2325

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

2 participants