Skip to content

feat: YouTube Component#4732

Merged
istarkov merged 15 commits intomainfrom
youtube.staging
Jan 9, 2025
Merged

feat: YouTube Component#4732
istarkov merged 15 commits intomainfrom
youtube.staging

Conversation

@istarkov
Copy link
Copy Markdown
Contributor

@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

@kof kof changed the title feat: Youtube control feat: YouTube Component Jan 8, 2025
@kof
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Reference #1747

@kof
Copy link
Copy Markdown
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

@istarkov istarkov closed this Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ability to set iframe title on Vimeo for accessibility Vimeo preview image has optimize disabled by default Youtube Component

3 participants