Skip to content

Picture in Picture#7445

Open
djblu2003dk-tech wants to merge 31 commits into
Chocobozzz:developfrom
djblu2003dk-tech:PIP
Open

Picture in Picture#7445
djblu2003dk-tech wants to merge 31 commits into
Chocobozzz:developfrom
djblu2003dk-tech:PIP

Conversation

@djblu2003dk-tech
Copy link
Copy Markdown
Contributor

Description

Changed the client and embedded player to include a Picture in Picture icon that allows for the player to pop out.

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this PR does not update server code
  • 🙋 no, because I need help

token.destroy()
.catch(err => logger.error('Cannot destroy token when revoking token.', { err }))
try {
await token.destroy()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you explain this change?

# Your S3 provider must support virtual hosting of buckets as PeerTube doesn't support path style requests
endpoint: '' # 's3.amazonaws.com' or 's3.fr-par.scw.cloud' for example
# If your S3 provider does not support virtual host style requests, enable path style requests
force_path_style: false
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is there a reason you moved these lines?


this.player.contextMenu(this.getContextMenuOptions())
if (this.isInIframe()) {
// Disable custom and native context menus in embeds.
Copy link
Copy Markdown
Owner

@Chocobozzz Chocobozzz Feb 24, 2026

Choose a reason for hiding this comment

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

From your other comment (https://github.com/Chocobozzz/PeerTube/pull/7396/changes#r2809262612), it seems you can remove this

this.controlText(isActive ? 'Exit picture-in-picture' : 'Picture-in-picture')
}

private buildPopoutUrl (embedUrl: string) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is not used?

}
}

private getPopupPlacement () {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same here

margin-right: auto;
}

// Always show PiP button in embed, even for small fixed-size iframes.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think it's a good idea, because it can break control bar on very small screens and it displays the icon on non supported web browsers

@Chocobozzz
Copy link
Copy Markdown
Owner

Thank you, I added some commits to fix conflicts and use another pip icon.

@Chocobozzz
Copy link
Copy Markdown
Owner

Can you rebase/merge develop now DVR is merged?

@djblu2003dk-tech
Copy link
Copy Markdown
Contributor Author

Yes, I'll work on this later this month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants