use flex-box to calculate time rail width #2261
Conversation
|
Thanks for this PR. However, we have to support IE9+ and those browsers don't support flexbox. If you can restrict flexbox on IE9 and making sure your change will work on those version of IE without increasing the size of code a lot I will merge this. Any questions do not hesitate on contacting me. |
|
@rafa8626 Alright, thanks for your feedback. If you still plan to support IE9, I would suggest keeping this change on hold an set a tag for a newer milestone. |
|
Sounds good to me. I'll create a separate branch for your change and keep it until I discuss this with John Dyer and we have an agreement on what we should support in the next release. Thanks |
|
FYI, I may not need a separate branch. I'll have an answer soon about browser support. So I'll keep this PR open while this happens. Thanks for this PR, since I also consider it very helpful |
|
Thank you Rafael. You are very welcome. |
# Conflicts: # src/css/mediaelementplayer.css
|
Ok I have the approval of going with flexbox for this. However can you actually turn the whole control bar to flexbox to reduce more code? If you have any other improvements that require flexbox please put them on this PR and I'll test them. Such good news! |
|
Alright, thanks for your feedback. I will work on this shortly. Which browsers will you support now on? So I can setup Autoprefixer |
|
I believe we'll be fine with IE11+, Chrome, Safari, FF, iOS 8+, android 4+, MSEdge. Even IE10 should be fine since it supports flexbox but let me know your thoughts. |
|
Any updates on this? |
|
@rafa8626 Sorry Rafael, I was quite busy this week. |
|
No worries; thanks for keep working on this. I'll check this today on MS Edge and IE11 and I'll give you an update. Did you test this on Android and iOS? |
|
I've tested it on Apple iPhone simulator. iOS looks fine. |
|
Ok I'll take a look at Android and IE; I noticed that |
|
On IE11, Firefox, I see that there's a little black border on top of the Captions/Volume |
| left: -1000px; | ||
| margin: 3px 3px 0 5px; | ||
| position: absolute; | ||
| display: none; |
There was a problem hiding this comment.
We can't use this because it affects accessibility when users tab to select captions/chapters. This must be reverted to the way it was; the only change that needs to be made is visibility: visible instead of hidden and the following lines must be removed:
.mejs__captions-button > .mejs__captions-selector,
.mejs__chapters-button > .mejs__chapters-selector {
visibility: visible;
}
|
I think I can take care of the line there. I just need to know if we can remove the width of the rail. |
|
Updating |
| @@ -1,7 +1,6 @@ | |||
| /* Accessibility: hide screen reader texts (and prefer "top" for RTL languages). | |||
| Reference: http://blog.rrwd.nl/2015/04/04/the-screen-reader-text-class-why-and-how/ */ | |||
| .mejs-offscreen { | |||
| clip: rect(1px, 1px, 1px, 1px); /* IE8-IE11 - no support for clip-path */ | |||
| clip-path: polygon(0 0, 0 0, 0 0, 0 0); | |||
There was a problem hiding this comment.
Update to latest version of http://blog.rrwd.nl/2015/04/04/the-screen-reader-text-class-why-and-how/
src/css/mediaelementplayer.css
Outdated
| @@ -293,6 +295,9 @@ Reference: http://blog.rrwd.nl/2015/04/04/the-screen-reader-text-class-why-and-h | |||
| /* Start: Progress Bar */ | |||
| .mejs__time-rail { | |||
| direction: ltr; | |||
| -webkit-box-flex: 1; | |||
| -ms-flex-positive: 1; | |||
| flex-grow: 1; | |||
| width: 200px; | |||
There was a problem hiding this comment.
Remove the width to ensure 100% flexibility on small screens
| } | ||
|
|
||
| .mejs-captions-selector-label, | ||
| .mejs-chapters-selector-label { | ||
| cursor: pointer; | ||
| float: left; |
There was a problem hiding this comment.
This is OK to leave in place
|
I left all my comments. Let me know if you have any questions, but if those are addressed we will be covering every target and I will merge this. Thanks |
|
@marcobiedermann If you are OK I'll proceed to do the changes and merge this. A bigger testing is happening right now and I'd like to integrate this as part of the oncoming release |
|
@rafa8626 Sure, go ahead. Thank you very much for your feedback. I am currently at work and could a look at this after I am back home. But if you are going to handle this I am absolutely fine :) |
|
PR merged. Thanks |
Instead of calculating the time rail width via JavaScript, you could make use of the flex-box
flex-growproperty.Because flex-box has pretty good support meanwhile, I think this would simplify some calculations which would make the overall plugin a bit faster.