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

'Fit To Window' Player button #1798

Merged
merged 5 commits into from
Oct 21, 2023
Merged

Conversation

D-Rekk
Copy link
Contributor

@D-Rekk D-Rekk commented Oct 20, 2023

New button to toggle 'Fit To Window' player size.

This is a 'Fit To Window' toggle (part of #1445).

  • Allows to quickly swap the current Player Size to Old Alternative – Fit To Window
  • Works in both Default View and Theater mode. Tested with Comments -> Sidebar enabled
  • Icon stolen taken from lucide-icons IIRC

There's possibly bugs with other settings I haven't tested yet

FitToWinToggle

@D-Rekk D-Rekk added 🧩Plan ready Solution or some specification noted; To-Do; steps for implementation (+raw brainstorming too maybe) UI (UX) about our UI (UX) Player-Button Feature should only take one click/impulse to toggle/tune labels Oct 20, 2023
@ImprovedTube ImprovedTube merged commit d59edf9 into code-charity:master Oct 21, 2023
@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 21, 2023

hey! <3 great!

player size:

  • 1.) Fit to window had the goal to increase the video width & height (as much as possible, in ratio)
  • 2.) Full window, meant to do the same - but also stretch the player beyond the video's ratio, just to have the player's black background everywhere. (renamed it: Full height because that's the correct description for now)
  • later we made them run without theater-mode, just in case width becomes available, when the user hides the sidebar.
    • bug: this video width doesn't increase with hidden sidebar anymore.
    • also, both feature never removed the page margins left & right (to win extra width for the video)
      • So Max width adds CSS for this too
        • However a little code is missing to ensure that the controls bar also increases in width (might stays static in width )
          • Max width is based on a copy of 2. Full height, just because in this combination it would also stretch the controls bar maybe, while combined with "1." it didn't seem to at all.
          • So instead, to make sense, we need to call the features:

            1.) max. video width "Fit to page"
            options to select: [always | player-button #556]


            - Q: maybe you know how to do this after you already cared so much for player size?

            2.) Full Player height
            options to select: [in theater-mode | always | can keep a button too, yet different icon with vertical arrow?]

            - ( + and more options for the youtube header: hide in theater mode|hover in theater mode )

Previously, Player size bugs: #1602


  • aand this deserves a shortcut shortcut.js and/or chrome://extensions/shortcuts) (just like several existing & future features )

@ImprovedTube
Copy link
Member

ImprovedTube commented Oct 29, 2023

reverting for troubleshooting for today ( will even do that when it seems the chance is less than 1% that a PR is any related to current issues, if those issue are big enough.)

  • undefined css values will can remove the player
    height: calc(var(--it-player-size)

    • so our code rather needs height: calc(var(--it-player-size, 100vh)
  • the following CSS should be conditional(?)

@D-Rekk

  • if you are convinced about the rest i re-revert already

  • the only bug i found is the player disappearing (undefined size) when i click the your button twice

ImprovedTube added a commit that referenced this pull request Oct 29, 2023
ImprovedTube added a commit that referenced this pull request Oct 29, 2023
opacity: 0.85,
position: "right",
onclick: function () {
let previousSize = ImprovedTube.storage.player_size === "fit_to_window" ? "do_not_change" : ImprovedTube.storage.player_size;
Copy link
Contributor Author

@D-Rekk D-Rekk Oct 29, 2023

Choose a reason for hiding this comment

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

I didn't get any bug you described until I remembered the CSS trick we added while working on the Comments -> Sidebar feature #1681. Basically "do_not_change" doesn't get applied by default, only when users change the Player Size at least one time. I should add a nullish operator here:
let previousSize = ImprovedTube.storage.player_size === "fit_to_window" ? "do_not_change" : (ImprovedTube.storage.player_size ?? "do_not_change")

Copy link
Member

Choose a reason for hiding this comment

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

thanks, oh, right - and we / somebody can work on that (#1685)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧩Plan ready Solution or some specification noted; To-Do; steps for implementation (+raw brainstorming too maybe) Player-Button Feature should only take one click/impulse to toggle/tune UI (UX) about our UI (UX)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants