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.

Sign up for GitHub

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

Jump to bottom

Display Youtube player in notice when youtube link found #760

Merged
merged 3 commits into from
Mar 11, 2021
Merged

Display Youtube player in notice when youtube link found #760

merged 3 commits into from
Mar 11, 2021

Conversation

gregoirelacoste
Copy link
Member

I added on the Notice component the possibility of recognizing youtube URLs and replacing them with the player of the corresponding video. For that I have:

  • created a getMessageWithYoutubePlayer file which takes the initial children (string) from the "Message" component and transforms it via a Regex by adding the youtube iframe, or returns nothing if the regex does not match
  • imported this function into the "Message" component
  • added a defautlMessageWithYoutubeVideo in generateNotice
  • everything is visible in the storybook: http://localhost:6007/?path=/story/extension-notice-details--with-youtube-video

@JalilArfaoui
Copy link
Member

@gregoirelacoste This is why it does not work : https://stackoverflow.com/questions/36337086/my-youtube-video-wont-show-in-iframe/36337328

You need to change any https://www.youtube.com/watch?v=XXX url to https://www.youtube.com/embed/XXX in the iframe

@gregoirelacoste
Copy link
Member Author

@JalilArfaoui I just tested that's it

@gregoirelacoste gregoirelacoste force-pushed the feature/youtubePlayerOnABulle branch from 6578c8d to 36d91a9 Compare December 28, 2020 16:34
@gregoirelacoste gregoirelacoste self-assigned this Dec 28, 2020
@lutangar
Copy link
Member

Hey @gregoirelacoste sorry for the late review!

I would rather keep Message as a dumb component and move the call to getMessageWithYoutubePlayer in NoticeDetails component.
What do you think @JalilArfaoui ? It would also mean to add the call to NoticePreview as well...

Also to expect a validation from the UI/Product team (maybe from @newick or @MaartenLMEM) could you provide a screenshot of the result?

Thanks.

@JalilArfaoui JalilArfaoui added the feature ✨ either feature request (issue) or new feature (PR) label Jan 12, 2021
@lutangar lutangar linked an issue Jan 13, 2021 that may be closed by this pull request
Youtube Vidéo embedded in notice #759
Closed
@lutangar
Copy link
Member

@JalilArfaoui can we consider that @gregoirelacoste won't fix these little issues?

@gregoirelacoste
Copy link
Member Author

hi @lutangar sorry for the late answer, please find here the screenshot of the feature :
image

@lutangar
Copy link
Member

lutangar commented Jan 27, 2021 •
edited
Loading

Thanks @gregoirelacoste ! @newick @MaartenLMEM could you review this?

@newick
Copy link
Member

newick commented Jan 28, 2021

It seems to work perfectly @lutangar !
thx @gregoirelacoste 🎁

@lutangar
Copy link
Member

lutangar commented Feb 4, 2021 •
edited
Loading

@newick I noticed the 👎 is little bit off on this screenshot, could you fix this?
I just reassigned this PR to you.

newick added a commit that referenced this pull request Feb 4, 2021
@newick
fix(Feedbacks): moves feedbacks component to the left, issue #760
87b78b3
@lutangar lutangar removed their request for review February 4, 2021 17:31
@lutangar
Copy link
Member

lutangar commented Feb 4, 2021

@JalilArfaoui re-review plz I fixed a few things

@lutangar lutangar force-pushed the feature/youtubePlayerOnABulle branch from 1275155 to a2a0ad2 Compare February 4, 2021 17:34
@lutangar lutangar requested a review from a team February 9, 2021 15:14
@christpet
Copy link
Contributor

Looks like this is all set right @lutangar @JalilArfaoui ? If yes let's merge!

@lutangar lutangar assigned lutangar and unassigned gregoirelacoste Mar 11, 2021
@lutangar
Copy link
Member

I suppose this isn't critical, but I'm the one who finished the job, so mightn't be the best judge, and no one else gave a review on since then... I'm adding @gregoirelacoste as a reviewer!

@lutangar
Copy link
Member

@gregoirelacoste can't add you as a reviewer since you were the original author... let's merge this and move on...

@lutangar lutangar merged commit 979c343 into master Mar 11, 2021
@lutangar lutangar deleted the feature/youtubePlayerOnABulle branch March 11, 2021 16:08
lutangar pushed a commit that referenced this pull request Mar 11, 2021
@newick @lutangar
fix(Feedbacks): moves feedbacks component to the left, issue #760
c11f7f5
bmenant added a commit that referenced this pull request Mar 11, 2021
@bmenant
chore: release 3.63.0
04325ef 
# [3.63.0](v3.62.0...v3.63.0) (2021-03-11)

### Bug Fixes

* **Feedbacks:** moves feedbacks component to the left, issue [#760](#760) ([c11f7f5](c11f7f5))
* Make preview work with a youtube video ([979c343](979c343))

### Features

* display Youtube player in notice when youtube link found ([2b6bd31](2b6bd31))
@bmenant
Copy link
Member

bmenant commented Mar 11, 2021

🎉 This PR is included in version 3.63.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JalilArfaoui JalilArfaoui Awaiting requested review from JalilArfaoui

Assignees

@lutangar lutangar

@newick newick

Labels
feature ✨ either feature request (issue) or new feature (PR) released tested ✅
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Youtube Vidéo embedded in notice
6 participants
@gregoirelacoste @JalilArfaoui @lutangar @newick @christpet @bmenant