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

#DearInternet - allow video embed only for videos on www.youtube.com #5923

Merged
merged 10 commits into from Dec 22, 2020

Conversation

mmmavis
Copy link
Collaborator

@mmmavis mmmavis commented Dec 17, 2020

Will assign code reviewer once this passes design review.


Allow video embed only for videos on https://www.youtube.com/embed and update help text


Closes #5901

Since it seems like we only have that one YouTube video we wanna show (so far, at least) and that this campaign is only till next Jan, I went with the simplest solution

  • If a letter includes a valid YouTube embed video url: embed the video (don't show image even if the letter contains an image)
  • If a letter includes a video url: regardless what the url is, display a line: Video url: [link to video url]
  • If a letter includes an image and doesn't contain a valid YouTube embed video url: show image

See test use cases on https://foundation-s-issue-5901-nsn6o5.herokuapp.com/en/campaigns/dearinternet/

@mofodevops mofodevops temporarily deployed to foundation-s-issue-5901-nsn6o5 December 17, 2020 22:30 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-issue-5901-nsn6o5 December 17, 2020 22:51 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-issue-5901-nsn6o5 December 17, 2020 23:31 Inactive
@mmmavis mmmavis marked this pull request as ready for review December 18, 2020 00:53
@mmmavis mmmavis temporarily deployed to foundation-s-issue-5901-nsn6o5 December 18, 2020 00:53 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-issue-5901-nsn6o5 December 18, 2020 01:51 Inactive
Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

The use cases and styling look good!
The only thing that stands out is the text "Video url:" isn't changeable. Thinking ahead perhaps it would be more flexible if the label wasn't there or changeable so for use cases where we'd want to include a URL (to a website, image source etc.) it'd work too and not necessarily limited to video? I'd check with Audrey if we want label copy.

@mmmavis
Copy link
Collaborator Author

mmmavis commented Dec 18, 2020

@sabrinang

The only thing that stands out is the text "Video url:" isn't changeable. Thinking ahead perhaps it would be more flexible if the label wasn't there or changeable so for use cases where we'd want to include a URL (to a website, image source etc.) it'd work too and not necessarily limited to video? I'd check with Audrey if we want label copy.

Good point. I'll just remove the label copy so that line stays unlabelled. My availability will be limited next week so I just wanna wrap up all my assigned dev work today.

@mmmavis mmmavis temporarily deployed to foundation-s-issue-5901-nsn6o5 December 18, 2020 18:58 Inactive
@sabrinang sabrinang self-requested a review December 18, 2020 19:03
Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

Thanks for removing the label, that will do! 👍

@mmmavis mmmavis requested a review from Pomax December 18, 2020 19:29
@mmmavis mmmavis temporarily deployed to foundation-s-issue-5901-nsn6o5 December 21, 2020 18:51 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-issue-5901-nsn6o5 December 21, 2020 21:54 Inactive
@mmmavis
Copy link
Collaborator Author

mmmavis commented Dec 22, 2020

@Pomax do you have the bandwidth to review this today? We are hoping to get this landed before the holiday shutdown.

@Pomax Pomax temporarily deployed to foundation-s-issue-5901-nsn6o5 December 22, 2020 19:19 Inactive
Comment on lines +31 to +34
"Note that video embed will show only when the url is a valid YouTube video embed url. "
"Go to your YouTube video and click “Share,” "
"then “Embed,” and then copy and paste the provided URL only. "
"For example: https://www.youtube.com/embed/s7OD5BgFrVM",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

code looks good

@Pomax Pomax merged commit 339cdaf into master Dec 22, 2020
@Pomax Pomax deleted the issue-5901-dearinternet-youtube-embed branch December 22, 2020 19:30
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.

#DearInternet - allow YouTube video embed
4 participants