Feature/video/aime video player #156 by rin · Pull Request #163 · aimementoring/blueprint

rin · 2020-01-27T11:29:26Z

resolves #156

Currently this is working locally with YoutTube and Vimeo, partially AIME Vids.
You can see all props and functions on BP AimeVideoPlayer I have listed them in the Markdown.

I have also added a very basic Modal component just to be used with video component to render a Theatre-ish mode. this can be used with other things also.

Only for AIME videos, for some reason withModal onClick event to hide the modal is also overlaying the <video> element I have not been able to figure out why yet.

Another interesting thing with AIME video is if i just add the 1 required prop url the preview image of the video and the custom play button do not display.
I am not 100% sure why not, but this might be outside the realm of ReactPlayer to fix, it might be an aws thing really don't know yet.

I will be reviewing again previous convos around this player to keep working on functionality, but please let me know what is missing here if you notice it.
Any feedback will be greatly appreciated, it is my first attempt adding to Blueprint!!

@rin Couldn't request your review as it is your pr haha so the tag is the request 😄

Note: YouTube is as minimal as possible r.e controls branding, they are trying to provide consistent UI, platform, brand recognition, where ever the YT videos are embedded.

Volume control was added as standard volume for FF and CHROME where different Chrome was really loud for me now they are both the same.

AutoPlay is needed to allow one click play, but thats all it is used for it is set to false by default on load.

I think configs are minimal but thats just me let me know what can be removed.

it is possible to add subtitles which look relativly easy to add and can just be passed as strings from aws for example.

There are other items that are known to me, but cant think of them right now.
I added the label component dev ready but i don't really k now what that means?

Staging to Master

rin · 2020-01-27T11:29:48Z

@bronz3beard I created a draft pull request with your changes for easier review!

rin · 2020-01-27T11:44:28Z

-      backgroundVimeo,
-    } = this.props;
+  useEffect(() => {
+    document.body.style.overflow = showModal ? "hidden" : "auto";


This works, but usually you would just use showModal in your render function … or in your case, in what your functional component returns, and there add a class based on whether showModal is true or false.

A little bit of vanilla JS never hurt anyone right? haha
You are 100% correct a much better approach!

So i looked into this and actually it is the best way I can think of to manage hiding overflow-y in theatre mode for the body elem.
I left some comments in modal comp.
If there is a better way please show me 😃 I cant think of it.

rin · 2020-01-27T11:48:28Z

I really liked that you got the custom play button in there.
I also think it could be a good idea to have a Modal component, but we should talk about it with @kbardi !

rin · 2020-01-27T11:55:03Z

+export default class AimeVideoPlayer extends PureComponent {
+  static propTypes = {
+    url: PropTypes.string.isRequired,
+    controlsVimeo: PropTypes.bool,


I haven't had a closer look at the video player, I just wanted to suggest we keep all this configuration stuff to a minimum … so, unless we are sure we need it, I wouldn't even add all these props. Especially not the autoplay one, because I hope we will never do it!

I think it also makes things a little complicated to add configurations per player …? Happy to talk about this in a call!

I haven't had a closer look at the video player, I just wanted to suggest we keep all this configuration stuff to a minimum … so, unless we are sure we need it, I wouldn't even add all these props. Especially not the autoplay one, because I hope we will never do it!

I think it also makes things a little complicated to add configurations per player …? Happy to talk about this in a call!

Hey @rin ,

Yeah this is just getting to know the player right now and figuring out which props are essential, the auto play will be needed so we can have just one click of the play button.
Still looking at if that's the best solution.
Happy to jump on a call and bounce some ideas around 😃

Hey @rin if you have any suggestions on what to remove I will happily look into it. right now it feels like what we have is what we need but that's just my perspective 😄

camposcristian · 2020-02-07T00:03:55Z

Just a note to myself that would love to test how to integrate this in the portal soon :)

kbardi

It looks good, but I left some comments. I'm worry about the modal and using plain javascript to modify a class in the body

kbardi · 2020-03-04T21:00:53Z

+
+  return (
+    <div className={styles[`theme-${theme}`]}>
+      {!showModal && (


Also I think we can improve a little bit this jsx to reduce some repetitions and make a smaller file

kbardi · 2020-03-04T21:06:29Z

+  useEffect(() => {
+    // This is manipulating the parent DOM of the react virtual DOM, so I think its going to be OK for what we need?
+    // we would have to hide overflow for the body and app and then make them fill the page and then adjust the react elements to be able to controll overflow-y in css, I think.
+    document.body.style.overflowY =


not sure if it is a good idea, because for example on website we cannot access easily to the dom when we are in the server side, so it won't work there :(

I think we can run a check for document, and it is also only available when the vidoe is ready, which should be client side right?
Also it could be done with css and some extra divs but I think thats probably too complex just for hiding body scroll when modal is active.

I am happy to do anything with that we dont need to hide scroll, I'm open to any suggestion. 👍

rin · 2020-03-16T10:43:14Z


-*NB: Blueprint will be linked until you run…`yarn unlink “aime-blueprint”`*
-
+*NB: Blueprint will be linked until you run…`yarn unlink` in your `blueprint` terminal **as well as** `yarn unlink “aime-blueprint”` in your local project (`website` or `portal`)*


…om/aimementoring/blueprint into feature/video/aimeVideoPlayer-#156

rin · 2020-03-19T14:53:28Z

+  return (
+    <div className={styles[`theme-${theme}`]}>
+      {!showModal && (
+        <div className={styles.playerContainer}>


we should add containerClassName here (it's passed as a prop)

rin · 2020-03-19T16:02:07Z

+    imageUrl === "" ? lightMode : !showModal && imageUrl;
+
+  const backGroundStyle = {
+    background: `url(${imageUrl})`,


should be url('${imageUrl}')

rin · 2020-03-19T16:02:22Z

+AimeVideoPlayer.propTypes = {
+  loop: PropTypes.bool,
+  mute: PropTypes.bool,
+  title: PropTypes.bool,


rin · 2020-03-19T16:02:39Z

+AimeVideoPlayer.defaultProps = {
+  loop: false,
+  mute: false,
+  title: false,


title: ''

rin · 2020-03-19T16:57:37Z

+.playerContainer {
+  width: 100%;
+  height: 100%;
+  display: flex;


remove this

rin · 2020-03-19T16:57:51Z

+  align-items: center;
+  justify-content: center;
+
+  .playerBoarder {


we don't need this anymore because our style looks different now

rin · 2020-03-19T16:58:15Z

+      width: 100%;
+      color: #000;
+      position: absolute;
+      transform: translate(-50%, -3%);


if we dont use flex above, we can drop the positioning and transforming here

lulen11 and others added 8 commits January 16, 2020 12:10

Merge pull request #143 from aimementoring/staging

38656de

Staging to Master

updating readme

d277198

updating readme

122f450

update generated library

7ba27c2

2.2.0

9a27717

WIP AimeVideoPlayer Comp

3ad4717

updated settings json

6e8a7b3

added modal, video functinality for vimeo and styles

1e28a33

rin commented Jan 27, 2020

View reviewed changes

Comment thread src/lib/components/AimeVideoPlayer/AimeVideoPlayer.js Outdated

rin commented Jan 27, 2020

View reviewed changes

bronz3beard mentioned this pull request Jan 28, 2020

Configure video player #31

Closed

bronz3beard added 3 commits January 28, 2020 17:55

added some YT props refined Vim props updated styles

78e2c3a

updated styles inline with figma WIP added more YT functionality

138bcd4

fixed volume prop for ReactPlayer

07600aa

bronz3beard requested review from charliemckenzie, kbardi and lulen11 February 5, 2020 04:27

bronz3beard self-assigned this Feb 5, 2020

bronz3beard added component-dev-ready Component has design and is ready to be coded component-proposed Proposed Component labels Feb 5, 2020

bronz3beard added 2 commits February 5, 2020 14:41

WIP adding AIME vid functionality

af6616a

forgot to test component and build lib

92924d7

update react-player some minor code changes and changes to .md

533743e

camposcristian marked this pull request as ready for review February 12, 2020 08:46

fix modal and mp4 controls

0c49e39

lulen11 reviewed Feb 19, 2020

View reviewed changes

Comment thread src/lib/components/AimeVideoPlayer/AimeVideoPlayer.module.scss

kbardi suggested changes Mar 4, 2020

View reviewed changes

added some updates inline with review

d5cdf36

rin commented Mar 16, 2020

View reviewed changes

rin and others added 5 commits March 16, 2020 11:46

Fix: Update tests.

330b535

minor changes added hls links

39797c1

Merge branch 'feature/video/aimeVideoPlayer-#156' of https://github.c…

b78d628

…om/aimementoring/blueprint into feature/video/aimeVideoPlayer-#156

Chore: camelCaseAllTheThings.

b19a0eb

update tests and move custom play icon to video player file

70e48db

lulen11 mentioned this pull request Mar 17, 2020

Enhance Video Component #24

Closed

rin commented Mar 19, 2020

View reviewed changes


		NB: Blueprint will be linked until you run…`yarn unlink “aime-blueprint”`

		NB: Blueprint will be linked until you run…`yarn unlink` in your `blueprint` terminal as well as* `yarn unlink “aime-blueprint”` in your local project (`website` or `portal`)*

Conversation

rin commented Jan 27, 2020 • edited by bronz3beard Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rin commented Jan 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rin commented Jan 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camposcristian commented Feb 7, 2020

Uh oh!

Uh oh!

kbardi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rin Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rin commented Jan 27, 2020 •

edited by bronz3beard

Loading

rin Mar 19, 2020 •

edited

Loading