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.

Sign up for GitHub

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

Jump to bottom

Move fonts out of core repo #2223

Closed
wants to merge 3 commits into from
Closed

Move fonts out of core repo #2223

wants to merge 3 commits into from

Conversation

mmcc
Copy link
Member

@mmcc mmcc commented Jun 1, 2015

There's now a font build pipeline: videojs-font

@mmcc mmcc added this to the v5.0.0 milestone Jun 1, 2015
@gkatsev
Copy link
Member

gkatsev commented Jun 1, 2015

You mean https://github.com/videojs/font?

@gkatsev
Copy link
Member

gkatsev commented Jun 1, 2015

Ugh, the diff for the grunt.js file is horrible.

@mmcc
Copy link
Member Author

mmcc commented Jun 1, 2015

Ah yeah, forgot to revert the formatting change. I have my editor set to
auto format on save and meant to disable it before messing with core.

On Mon, Jun 1, 2015 at 9:04 AM Gary Katsevman notifications@github.com
wrote:

Ugh, the diff for the grunt.js file is horrible.

—
Reply to this email directly or view it on GitHub
#2223 (comment).

heff
heff reviewed Jun 1, 2015
View reviewed changes
src/css/_variables.scss
@@ -4,7 +4,6 @@ $secondary-text: #F4A460; // currently just used for visited links in errors and
$primary-bg: #000;
$secondary-bg: lighten($primary-bg, 35%);

$icon-font-family: 'VideoJS';
Copy link
Member

Choose a reason for hiding this comment

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

Should we not still set this here anyway? How might someone override it with their own? If that's a real use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

If someone was overriding it with there own, they'd probably want to override all of the icons partial basically (at least that was my thought).

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that makes sense

@heff
Copy link
Member

heff commented Jun 1, 2015

Should be good to go after the grunt file cleanup.

@heff
Copy link
Member

heff commented Jun 2, 2015

Looks good. Pulling in.

@mmcc mmcc closed this in dac91a7 Jun 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mmcc @gkatsev @heff