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

rework build to seperate out ie8 specific sass #3380

Closed
wants to merge 2 commits into from
Closed

rework build to seperate out ie8 specific sass #3380

wants to merge 2 commits into from

Conversation

erikyuzwa
Copy link
Contributor

Description

  • This is in reference to issue video-js.css corrupt #3013
  • Slice out some identified lines of sass specific to IE8 and create its own separate .css file if needed by developers

Specific Changes proposed

  • added src/css/ie8.scss to capture all ie8 specific sass.
  • moved out some lines from src/css/components/_control-bar.scss which were ie8 specific.
  • added some necessary modifications to the grunt build task to support this.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@gkatsev
Copy link
Member

gkatsev commented Jun 20, 2016

Moving the IE8 stuff to another sass file, unfortunately, won't fix the issue. It needs to be turned into a plain old css file (so, sass won't turn the special characters into utf8/unicode characters).
Also, we don't want to create any breaking changes in patch or minor releases. Would be nice if this extra file can just be appended together with the file generated from sass.

@gkatsev
Copy link
Member

gkatsev commented Jun 20, 2016

Also, wanted to thank you for getting started on this. Let me know if you can get to the changes above, otherwise, I can probably pull your changes in and make the other changes as well.

@gkatsev gkatsev added minor This PR can be added to a minor release. It should not be added to a patch release. needs: updates labels Jun 20, 2016
@erikyuzwa
Copy link
Contributor Author

gotcha. Thanks for the extra clarifications @gkatsev - I should be able to take care of this. No worries at all.

@erikyuzwa
Copy link
Contributor Author

the only thing outstanding is that the unminified src/css/ie8.css isn't being appended to build/temp/video-js.css

@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

Having the temp file not include the ie8 piece is fine, I think.

@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

While I think that build/temp/video-js.css is fine if it's missing, it does need to be in dist/video-js.css. Looking into what needs to change. Maybe we can still get it into build/temp/video-js.css anyway.

@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

Ah, we can use concat to do this. I'll make a PR for this against your PR.

@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

Made a PR for this, see https://github.com/erikyuzwa/video.js/pull/1
What it does is uses concat to add the ie8-only files into build/temp/video-js.css after the sass task compiles all the sass stuff into build/temp/video-js.css. Then we don't need the cssmin changes because the ie8 changes are already in the file.

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Jul 20, 2016
@gkatsev gkatsev added this to the 3.12 build-improvements milestone Aug 3, 2016
@misteroneill
Copy link
Member

Love this. LGTM.

@gkatsev
Copy link
Member

gkatsev commented Aug 5, 2016

landed in d86d4b2 and 1ff9f38.
Thanks @erikyuzwa for starting this!

@gkatsev gkatsev closed this Aug 5, 2016
@gkatsev gkatsev mentioned this pull request Aug 5, 2016
@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals needs: updates labels Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants