Skip to content

demo(youtube-player): add auto video resizing #17648

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

Merged
merged 1 commit into from
Nov 8, 2019
Merged

Conversation

Splaktar
Copy link
Contributor

@Splaktar Splaktar commented Nov 7, 2019

  • automatically resize the video to fit the screen
    • this was noticeable on mobile where the video would just get cut off
  • rename video to selectedVideo for clarity
  • fix TypeScript path mappings so that IDEs don't complain about module
  • use classes to style Material components as recommended

@Splaktar Splaktar added pr: merge safe P4 A relatively minor issue that is not relevant to core functions target: minor This PR is targeted for the next minor release labels Nov 7, 2019
@Splaktar Splaktar requested a review from jelbourn as a code owner November 7, 2019 20:42
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 7, 2019
@YourDeveloperFriend
Copy link
Collaborator

I wonder if this is highlighting a bigger problem with the component: most users would expect the youtube video to fill the width of the youtube-player element. Rather than forcing the user to explicitly set a width and height, I wonder if it should default to whatever the css style of the youtube-player element.

@Splaktar
Copy link
Contributor Author

Splaktar commented Nov 8, 2019

@YourDeveloperFriend I do think that there is a case for a fixed size API, but I agree that there also should be some kind of autoSize mode that fits the video automatically. It's not something that is trivial for many developers to do.

- automatically resize the video to fit the screen
  - this was noticeable on mobile where the video would just get cut off
- rename video to selectedVideo for clarity
- fix TypeScript path mappings so that IDEs don't complain about module
- use classes to style Material components as recommended
@Splaktar Splaktar force-pushed the youtube-player-demo branch from 242f19c to 5254796 Compare November 8, 2019 00:59
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

This reminds me that I want to make a StackBlitz of this demo running and link to it from here

@jelbourn jelbourn added docs This issue is related to documentation pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Nov 8, 2019
@mmalerba mmalerba merged commit b5146bf into master Nov 8, 2019
@Splaktar Splaktar deleted the youtube-player-demo branch November 8, 2019 23:32
@Splaktar
Copy link
Contributor Author

Splaktar commented Nov 9, 2019

@jelbourn here's a StackBlitz of this demo. Did you just want to link it from this PR or from the README of the YouTube Player component?

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation P4 A relatively minor issue that is not relevant to core functions target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants