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

refactor: remove IE8 specific changes #5041

Merged
merged 43 commits into from
Mar 23, 2018
Merged

refactor: remove IE8 specific changes #5041

merged 43 commits into from
Mar 23, 2018

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Mar 21, 2018

BREAKING CHANGE: remove IE8 specific JavaScript and CSS code. Remove Android 2.3 workaround.

TODO:

  • remove weird track workarounds for defineProperty
  • use defineProperties where appropriate
  • update vtt.js to remove weird workaround for defineProperty
  • update vtt.js to use defineProperties
  • remove IS_OLD_ANDROID canPlayType monkeypatching.
  • remove es3 preset
  • remove IE8 CSS hacks and workarounds.
  • remove videojs-ie8 dep
  • update sandbox files appropriately.
  • remove IE8 specific tests
  • remove IE8 checking in tests and just run them always.
  • fix broken tests
  • update videojs-fonts to remove ie8 stuff

@gkatsev
Copy link
Member Author

gkatsev commented Mar 21, 2018

The best part is that this change makes the min.gz file 0.16KB larger.

@@ -1,6 +1,5 @@
{
"presets": [
"es3",
["es2015", {
"loose": true
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we only do loose for IE < 11 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

we don't need loose mode but the filesize output from loose mode is smaller compared to regular.

@@ -70,7 +68,6 @@ export const CHROME_VERSION = (function() {
}
return null;
}());
export const IS_IE8 = (/MSIE\s8\.0/).test(USER_AGENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we should remove this code

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, maybe we should keep these, not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think I'm inclined to leave it out since we don't need any IE8 checks and we can always add it back in if necessary.

@brandonocasey
Copy link
Contributor

could also be good to mention in the title that this removes old android code as well.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 21, 2018

I'll add the android thing as part of the BREAKING CHANGE flag in the commit message.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 21, 2018

actually, I think the IE8 CSS stuff can come in a separate PR.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 21, 2018

same with the vttjs update.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 21, 2018

well, did the css changes. font stuff and vttjs stuff can and should come in separate PRs

@gkatsev gkatsev changed the title WIP: Remove IE8 refactor: remove IE8 specific changes Mar 21, 2018
@brandonocasey
Copy link
Contributor

brandonocasey commented Mar 22, 2018

CSS still in the branch that references IE < 11. If these are still valid we should remove/change the comments
https://github.com/videojs/video.js/blob/master/src/css/components/_control-bar.scss#L47
https://github.com/videojs/video.js/blob/master/src/css/components/_control.scss#L33
https://github.com/videojs/video.js/blob/master/src/css/components/_layout.scss#L106
https://github.com/videojs/video.js/blob/master/src/css/components/_layout.scss#L152
https://github.com/videojs/video.js/blob/master/src/css/components/_poster.scss#L19
https://github.com/videojs/video.js/blob/master/src/css/components/_progress.scss#L66
https://github.com/videojs/video.js/blob/master/src/css/components/_progress.scss#L95
https://github.com/videojs/video.js/blob/master/src/css/components/_progress.scss#L97
https://github.com/videojs/video.js/blob/master/src/css/components/_time.scss#L21
https://github.com/videojs/video.js/blob/master/src/css/components/_text-track.scss#L22
https://github.com/videojs/video.js/blob/master/src/css/components/_volume.scss#L4
https://github.com/videojs/video.js/blob/master/src/css/components/menu/_menu.scss#L25
https://github.com/videojs/video.js/blob/master/src/css/components/menu/_menu.scss#L34
https://github.com/videojs/video.js/blob/master/src/css/components/_modal-dialog.scss#L9

Do we need the -ms-filter here?
https://github.com/videojs/video.js/blob/master/src/css/components/_volume.scss#L68

Seems like we should manually run autoprefixer over some of this code. Mostly for this pr I am worries about the -ms- vendor prefix
https://github.com/videojs/video.js/blob/master/src/css/_utilities.scss

@brandonocasey
Copy link
Contributor

I think we use them in a lot of places internally, and new techs it could be useful

@gkatsev
Copy link
Member Author

gkatsev commented Mar 22, 2018

A bit hesitant to make changes to URL.js. Not touching events.js, I think the try/catch in a few places should stay just to be extra safe, the "ie6" SO link for getAbsoluteURL still applies, since it still uses the same technique.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 22, 2018

We have a few things here that related to IE and no media player, I'm inclined to leave the try/catches for them in. I'll update them to just mention IE in general, rather than IE9 or something.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 22, 2018

also, the two flash related items might be considered in a separate PR/issue.

@brandonocasey
Copy link
Contributor

brandonocasey commented Mar 23, 2018

More stuff

Not sure if we came to a conclusion, or if the comment just needs to be changed:

In tests/docs if we want to do them in this pr

docs

tests

@gkatsev
Copy link
Member Author

gkatsev commented Mar 23, 2018

Some of what you linked was already fixed. Most of what I haven't changed is because it isn't worth changing at least not for the PR because it involves unnecessary refactoring.

@brandonocasey
Copy link
Contributor

ok well the code still lgtm, I added check marks to my previous comment, feel free to check off what's done and I will move everything else into a separate issue.

@gkatsev gkatsev merged commit bc2da7c into master Mar 23, 2018
@gkatsev gkatsev deleted the remove-ie8 branch March 23, 2018 17:25
@gkatsev gkatsev mentioned this pull request Apr 23, 2018
gkatsev added a commit that referenced this pull request May 22, 2018
When having a video-js embed with a class attribute, as part of the
changes to remove old IE support (#5041), we overwrote our addition of
the video-js class when it was missing. Instead, we want to make sure
that we don't override the class names again since they are already set
up correctly.

Fixes videojs/http-streaming#100
gkatsev added a commit that referenced this pull request May 23, 2018
When having a video-js embed with a class attribute, as part of the
changes to remove old IE support (#5041), we overwrote our addition of
the video-js class when it was missing. Instead, we want to make sure
that we don't override the class names again since they are already set
up correctly.

Fixes videojs/http-streaming#100
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.

3 participants