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

feat(player.css): Fixed max-height constraint (fixes #1710) #1712

Merged
merged 3 commits into from
Jul 4, 2023

Conversation

HanzCEO
Copy link
Contributor

@HanzCEO HanzCEO commented Jul 3, 2023

Added overwrite to max-height property at #player-wide-container for 'Full Height' sizing.

@HanzCEO HanzCEO requested a review from ImprovedTube July 3, 2023 04:38
@HanzCEO HanzCEO added ...YouTube changed CSS Solution can be CSS ( /mostly - if any JS is needed, please specify) labels Jul 3, 2023
@PoorChameleon
Copy link

PoorChameleon commented Jul 3, 2023

Did you test this working with header set to hidden or hidden on video page? It looks like it won't work in that case.

It also might break non-theater mode

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 3, 2023

Did you test this working with header set to hidden or hidden on video page? It looks like it won't work in that case.

It also might break non-theater mode
@PoorChameleon

I have fixed that navbar issue in the last commit.
However, only in theater mode full_window (Full Height mode) will apply.

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 3, 2023

Maybe I misunderstood this Full Height sizing option. What should it do exactly @PoorChameleon?

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 3, 2023

I have tested my solution against all navbar display option from the extension. It works fine when combined with theater mode (both forced from extension or not)

@PoorChameleon
Copy link

PoorChameleon commented Jul 4, 2023

It does mostly work now. I can still find one use case where it breaks which is enabling forced theater mode and then manually pressing 't' to disable it in the current video the user is on.

You may be able to get away with just using html[data-page-type=video][it-player-size='full_window'] ytd-watch-flexy[theater] #player-wide-container as the selector, but I'm not sure.

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 4, 2023

@PoorChameleon alright, here it is

@ImprovedTube ImprovedTube merged commit e621542 into code-charity:master Jul 4, 2023
@ImprovedTube
Copy link
Member

Thank you! @HanzCEO ( are you on Discord too? if not i should start syncing both somehow. @D-Rekk )

@D-Rekk
Copy link
Contributor

D-Rekk commented Jul 4, 2023

@ImprovedTube this is what we had planned to do. The code is the same.
We also checked that the same style worked for "fit-to-window". Could've been added a few lines below.

What I don't understand is the --header-size variable in player.css

html[data-page-type=video]:not([it-header-position]) ytd-app,
html[data-page-type=video][it-header-position=normal] ytd-app,
html[data-page-type=video][it-header-position=static] ytd-app {
	--it-header-size: 56px;
}

html[data-page-type=video]:not([it-header-position]) ytd-watch-flexy:not([theater]),
html[data-page-type=video][it-header-position=normal] ytd-watch-flexy:not([theater]),
html[data-page-type=video][it-header-position=static] ytd-watch-flexy:not([theater]) {
	--it-header-size: 104px;
}

html[data-page-type=video]:not([it-header-position]) div#page,
html[data-page-type=video][it-header-position=normal] div#page,
html[data-page-type=video][it-header-position=static] div#page {
	--it-header-size: 50px;
}

html[data-page-type=video]:not([it-header-position]) div#page:not(.watch-wide),
html[data-page-type=video][it-header-position=normal] div#page:not(.watch-wide),
html[data-page-type=video][it-header-position=static] div#page:not(.watch-wide) {
	--it-header-size: 70px;
}
/* other quadruplets for hidden, full-height, fit-to-window, hover... */

When should we get the other values over the first base one? I was trying to debug why the player it's never reaching the full height of the page. The header never changes its height in fullscreen, theater or in wide-screen, yet the greatest value takes precedence. Perhaps some obsolete code?
The first value of these custom --header-position is always the correct one. Once adjusted, the height CSS to apply should be changed to height: calc(var(--it-player-size) - var(--it-header-size) - 24px )!important;


Also, should the full-height remove the padding-top?

With margin (actual)

Screenshot 2023-07-04 alle 19 23 20

Without margin

Screenshot 2023-07-04 alle 19 31 31

Without margin (hidden header)

Screenshot 2023-07-04 alle 19 32 04

Without margin (hidden header & fit-to-window)

Screenshot 2023-07-04 alle 19 44 08

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 4, 2023

Thank you! @HanzCEO ( are you on Discord too? if not i should start syncing both somehow. @D-Rekk )
@ImprovedTube
My discord: hanzceo

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 4, 2023

Also, should the full-height remove the padding-top?
@D-Rekk

Maybe i don't correctly understood what full-height mode is, would you mind to enlighten me a bit?

@D-Rekk
Copy link
Contributor

D-Rekk commented Jul 4, 2023

@HanzCEO we're referring to the same thing. It's "full-window" in the CSS and "full-height" in the tooltip

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 5, 2023

we're referring to the same thing. It's "full-window" in the CSS and "full-height" in the tooltip
@D-Rekk

I mean, the expected behavior from this mode. I'm not confused with its naming convention.

@D-Rekk
Copy link
Contributor

D-Rekk commented Jul 5, 2023

I mean, the expected behavior from this mode. I'm not confused with its naming convention.

It's to expand player to fill the viewport height (minus the header size and padding). Currently its not reaching the bottom part because there's a strange overlapping of variables. --it-header-size is greater than what it should be.
"Fit-to-window" would be the same but with transparent background IIRC🤔

@ImprovedTube
Copy link
Member

Perhaps some obsolete code?
The first value of these custom --header-position is always the correct one. Once adjusted, the height CSS to apply should be changed to height: calc(var(--it-player-size) - var(--it-header-size) - 24px )!important;

🤩 Pretty old at least (Didnt write it. Always wondered why Github wouldn't show the the original author and date of each line.) Looking foward to the/your next PR 🤫😁

Also, should the full-height remove the padding-top?

agree.

"Fit-to-window" would be the same but with transparent background IIRC🤔

This used to not increase player-height beyond the video height. (title stays visible. description & comments too on vertical screens) . Not sure why no longer.

Same issure with the first option "Max width" (Does this ones correctly stretch the player controls bar though? For me always except when setting itt)

@HanzCEO
Copy link
Contributor Author

HanzCEO commented Jul 9, 2023

Always wondered why Github wouldn't show the the original author and date of each line.
@ImprovedTube

It's called git blame but in GitHub there's a button called Blame upright the file content display

ImprovedTube added a commit that referenced this pull request Jan 12, 2024
feat(player.css): Fixed max-height constraint (fixes #1710)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Solution can be CSS ( /mostly - if any JS is needed, please specify) ...YouTube changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants