Fixes alignment problems in some videos #434

Closed

hackrush01 wants to merge 1 commit from video-fix into master

hackrush01 commented

2017-08-04 10:03:56 +02:00

(Migrated from github.com)

Couldn't break anything else after this fix, and the broken videos
were fixed. So I guess this is a proper fix, but not sure(if it was
this simple or not).

Fixes #419

Couldn't break anything else after this fix, and the broken videos _were_ fixed. So I guess this is a proper fix, but not sure(if it was this simple or not). Fixes #419

kauffj (Migrated from github.com) reviewed 2017-08-04 10:03:56 +02:00

tzarebczan (Migrated from github.com) reviewed 2017-08-04 10:03:56 +02:00

kauffj commented

2017-08-06 17:45:09 +02:00

(Migrated from github.com)

@hackrush01 if a line of code seems to make no sense, it's a good idea to check git blame to see if the PR that added it provides insight.

In this case, we see that it is this one: 99cb1a0a65

Now, despite writing that code just a month ago, I actually don't remember exactly what sizing issue it fixed. Perhaps videos less than the minimum height?

Regardless, it's clear that that code did something and we can't remove it without understanding what, or at least thoroughly testing different cases to ensure everything still works.

Welcome to the real-world problems of software engineering that aren't taught in school.

(Additionally, if we were remove absolute, we'd want to remove the other 2 lines.)

@hackrush01 if a line of code seems to make no sense, it's a good idea to check `git blame` to see if the PR that added it provides insight. In this case, we see that it is this one: https://github.com/lbryio/lbry-app/commit/99cb1a0a6502a2157fcd46b0c61017f75688a8b1 Now, despite writing that code just a month ago, I actually don't remember exactly what sizing issue it fixed. Perhaps videos less than the minimum height? Regardless, it's clear that that code did _something_ and we can't remove it without understanding what, or at least thoroughly testing different cases to ensure everything still works. Welcome to the real-world problems of software engineering that aren't taught in school. (Additionally, if we were remove absolute, we'd want to remove the other 2 lines.)

btzr-io commented

2017-08-09 04:20:40 +02:00

(Migrated from github.com)

Removing absolute works because video gets wrapped inside two divs and not directly in .video-embedded, but my guess it's that the controls should fit the main wrapper other wise looks weird,
unless you make .video-embedded transparent.

Current output:

   <div class="video-embedded">
     <div>
        <div>
           <video />
        </div>
     </div>
   </div>

It should be:

 <div class="video-embedded">
    <video />
  </div>

Removing absolute works because `video` gets wrapped inside two `divs` and not directly in `.video-embedded`, but my guess it's that the controls should fit the main wrapper other wise looks weird, unless you make `.video-embedded` transparent. Current output: ``` <div class="video-embedded"> <div> <div> <video /> </div> </div> </div> ``` It should be: ``` <div class="video-embedded"> <video /> </div> ```

btzr-io commented

2017-08-09 04:25:27 +02:00

(Migrated from github.com)

Also no idea why this was closed, it was going in the right direction 😛

Also no idea why this was closed, it was going in the right direction :stuck_out_tongue:

btzr-io commented

2017-08-09 04:33:10 +02:00

(Migrated from github.com)

Alternative don't wrap video inside those two divs ^^
and the current style should work 🙃

Alternative don't wrap `video` inside those two divs ^^ and the current `style` should work :upside_down_face:

btzr-io commented

2017-08-09 04:37:39 +02:00