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

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

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@Splaktar
Copy link
Member

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
@YourDeveloperFriend

This comment has been minimized.

Copy link
Collaborator

YourDeveloperFriend commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Member 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 Nov 8, 2019
Copy link
Member

jelbourn left a comment

LGTM

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

@mmalerba mmalerba merged commit b5146bf into master Nov 8, 2019
11 checks passed
11 checks passed
ci/angular: merge status All checks passed!
ci/circleci: api_golden_checks Your tests passed on CircleCI!
Details
ci/circleci: bazel_build Your tests passed on CircleCI!
Details
ci/circleci: build_release_packages Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: tests_browserstack Your tests passed on CircleCI!
Details
ci/circleci: tests_local_browsers Your tests passed on CircleCI!
Details
ci/circleci: tests_saucelabs Your tests passed on CircleCI!
Details
ci/circleci: view_engine_test Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
@Splaktar Splaktar deleted the youtube-player-demo branch Nov 8, 2019
@Splaktar

This comment has been minimized.

Copy link
Member 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.