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: YouTube Component #4732

Merged
merged 15 commits into from
Jan 9, 2025
Merged

feat: YouTube Component #4732

merged 15 commits into from
Jan 9, 2025

Conversation

istarkov
Copy link
Member

@istarkov istarkov commented Jan 8, 2025

Description

closes #1747
closes #3741
closes #3649

Suggestions:

  • - Remove autoplay, unpredictable in Chrome.

Additions:

  • - privacyEnhancedMode partially|maybe GDRP compliant

Steps for reproduction

  1. click button
  2. expect xyz

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • detailed review (read every line)
    • test it on preview

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 0000)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env file

@istarkov istarkov marked this pull request as ready for review January 8, 2025 08:59
@istarkov istarkov requested a review from kof January 8, 2025 08:59
@kof kof changed the title feat: Youtube control feat: YouTube Component Jan 8, 2025
@kof
Copy link
Member

kof commented Jan 8, 2025

image

I can tell you don't like them very much loll, please check everywhere it's called "YouTube" exactly like this.

Not Youtube, not You Tube

@kof
Copy link
Member

kof commented Jan 8, 2025

image

Something to think about, we had the same problem in a toast notification @TrySound recently wrote, can't find the PR, maybe we need markdown there if we don't want to support jsx in the tooltip?

@kof
Copy link
Member

kof commented Jan 8, 2025

  • Would be really nice for the List Type to have a select box with predefined values

@johnsicili
Copy link
Contributor

Good stuff Ivan! Just some minor stuff

Component desc is still Viemo:
Screenshot 2025-01-08 at 5 33 40 AM

In the toggles there are both "Keyboard" and "Disable Keyboard". Maybe they have different purposes but possibly a duplicate.

I think this is Vimeo's color. Probs best to use a YouTube color or no?
Screenshot 2025-01-08 at 5 41 29 AM

Preview image alt shows Vimeo
Screenshot 2025-01-08 at 6 07 43 AM

If you want to do any drive-bys (I would only recommend if they are very simple):

@johnsicili
Copy link
Contributor

Reference #1747

@kof
Copy link
Member

kof commented Jan 8, 2025

Ah yes, iframe title is something that is needed in particular when Autoplay is on, so the iframe is on the page already without user interaction

@kof kof requested a review from johnsicili January 9, 2025 03:01
@istarkov istarkov merged commit a09d12a into main Jan 9, 2025
27 checks passed
@istarkov istarkov deleted the youtube.staging branch January 9, 2025 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants