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

Font-face mixin – choose which formats to use #278

Merged
merged 2 commits into from Nov 4, 2014
Merged

Conversation

whmii
Copy link
Contributor

@whmii whmii commented Aug 1, 2014

I don't know if I'm missing something here, but does the font-face mixin assume you have .eot, .woff, .ttf and .svg files for your fonts? I would like to be able to use only .eot and .woff for example. Is this possible?

@imakewebthings
Copy link
Contributor

@jaydenseric
Copy link

The ability to set the formats would make a lot of sense. I have code inadvertently pointing to non existent SVG fonts of a few projects.

@plapier
Copy link

plapier commented Mar 14, 2014

Good point. It would be a useful feature in the mixin.

Perhaps an $exclude list could be passed to the mixin to drop and filetypes you don't need?

@include font-face(font, 'font/font', normal, $exclude: woff svg);

@KittyGiraudel
Copy link
Contributor

You should take it the other way around: a list of formats that are being used. Put them all as a default.

@mixin font-face(
  $font-family, 
  $file-path, 
  $weight: normal, 
  $style: normal, 
  $asset-pipeline: false, 
  $formats: eot woff ttf svg
) { 
  ... 
}

Excluding is kind of a weird thing. From my own experience, you'd better have a list of things that are being used rather than a list of things that are not. But that's just me.

@plapier
Copy link

plapier commented Mar 27, 2014

@hugogiraudel Good call. That's a better solution.

@tysongach tysongach added this to the 4.1.0 milestone Jul 30, 2014
@whmii
Copy link
Contributor

whmii commented Aug 1, 2014

I've added the option in this mixin. It is built so that you are able to pass in a list of filetypes. thanks to @hugogiraudel for the post on sass lists which is where a lot of this came from.

Also, to be able to iterate through the list of filetypes, I added a contains() function that will test if a list contains a defined value.

Here's a codepen of it so you can test it out → http://codepen.io/whmii/full/nfaBK/

ref: http://hugogiraudel.com/2013/07/15/understanding-sass-lists/

@KittyGiraudel
Copy link
Contributor

You are adding too much complexity to your contains function. Here is how you should be doing it:

@function contains($haystack, $needle) {
  @return not not index($haystack, $needle);
}

@tysongach
Copy link
Contributor

@whmii I’m confused. Was this PR re-opened by you? It seems like it was started by someone else, but the commits are from you, as if it was taken over, somehow.

@whmii
Copy link
Contributor

whmii commented Aug 2, 2014

@tysongach this was a bit of hub magic.
I used hub pull-request -i 278 -b thoughtbot:master -h thoughtbot:wm-fontface to convert the issue to a PR. it's OK but in the future i'll likely stick to just referencing the issue in the PR. also, it seems like this hub feature is going to be deprecated.

@whmii
Copy link
Contributor

whmii commented Aug 2, 2014

The contains() function has been updated with a more simple syntax in 6141fd5.

@whmii
Copy link
Contributor

whmii commented Aug 8, 2014

git status

awesome

@@ -0,0 +1,5 @@
// Test a Sass list to see if it contains a defined value

@function contains($list, $valie) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo $value instead of $valie 😉

@conchan
Copy link
Contributor

conchan commented Aug 29, 2014

@whmii good stuff!

$file-path,
$asset-pipeline,
$file-formats,
$font-url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after $font-url to match your @mixin font-face ( ... )

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, after seeing Reda's comment about the mixin 😉

Copy link

Choose a reason for hiding this comment

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

I'd go with no space to match the functions in the rest of the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave as is. This might be worth an update to the sass style guides for formatting sass maps and complex functions.

@kaishin
Copy link

kaishin commented Aug 29, 2014

Looking good to merge.

url('#{$file-path}.woff') format('woff'),
url('#{$file-path}.ttf') format('truetype'),
url('#{$file-path}.svg##{$font-family}') format('svg');
@if contains($formats, eot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should $formats be $file-formats?

@whmii
Copy link
Contributor

whmii commented Aug 29, 2014

Finally done. will rebase and merge once we get 1 more 👍 . A functional example can be found at http://codepen.io/whmii/pen/CHeoa/

@mkempe
Copy link

mkempe commented Sep 6, 2014

Hey there,

👍

Can we maybe add woff2 to the $formats-map:

$formats-map: (
  eot:   "#{$font-url}('#{$file-path}.eot?#iefix') format('embedded-opentype')",
  woff2: "#{$font-url}('#{$file-path}.woff2') format('woff2')",
  woff:  "#{$font-url}('#{$file-path}.woff') format('woff')",
  ttf:   "#{$font-url}('#{$file-path}.ttf') format('ttf')",
  svg:   "#{$font-url}('#{$file-path}.svg##{$font-family}') format('svg')"
);

but let the $file-formats argument as it is (eot woff ttf svg), because woff2 is to new and fancy.

Some links about the next generation woff:

https://gist.github.com/sergejmueller/cf6b4f2133bcb3e2f64a
http://sth.name/2014/09/03/Speed-up-webfonts

@mkempe
Copy link

mkempe commented Sep 23, 2014

Hey there,

just a note: Font Squirrel added support for WOFF2 to the Generator: https://twitter.com/FontSquirrel/status/509363706804076544

If you need a PR for woff2, just ping me.

@tysongach
Copy link
Contributor

@mkempe Great idea, thanks!

@whmii Good to add WOFF2, per comment above? I’ll add it in, if so…I’m readying v4.1.0.

@tysongach
Copy link
Contributor

@whmii Can you please add woff2 per comment above? And then squash your commits (probably down to 2 or 3—the addition of the contains should be its own)?

Use contains() to test if a list contains a defined value

The contains function works by attempting to find the index of a value within a list.
If it is unable to do so, the index function will return a false.
If it does return a value, that value is then converted in to a `true` via `not not`.
* Use maps in font-face for less code duplication
* Add font-url-prefixer function for rails pipeline
* Add font-source-declaration function to handle font src logic
* Add support for woff2
@whmii whmii merged commit 11b6a9f into master Nov 4, 2014
@whmii whmii deleted the wm-fontface branch November 4, 2014 18:54
@mkempe
Copy link

mkempe commented Nov 4, 2014

Many thanks!

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.

None yet

9 participants