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

Adds option to import and build fonts into projects #81

Closed
wants to merge 4 commits into from
Closed

Adds option to import and build fonts into projects #81

wants to merge 4 commits into from

Conversation

geotrev
Copy link

@geotrev geotrev commented Feb 19, 2018

Hola! Not sure if this has been considered in the past, but I think this would be a worthwhile addition to the pipeline.

As someone who has used the ZURB template many times over the years, I think the one unintuitive aspect of the setup is the inability to quickly add fonts for custom branding. I have gotten around this in the past by leveraging CDNs, but this is taxing for performance given it creates extra HTTP requests (much safer to build it into the pipeline and serve it directly, in my opinion).

Also, in searching around for solutions to this, I've found a variety of options but one that stuck out was Larmor27's solution over on the foundation-icon-fonts repo, so I abstracted it and created a simple implementation for discussion here.

Quick thoughts:

  1. The @font-face doesn't need to exist in src/assets/scss/components like I have here. In fact, I would see it doable to have a separate scss/fonts/ folder. I didn't do that here only because I could see a potential developer experience issue where folks throw actual font files into this folder despite the broader context being in the scss folder.
  2. I'm not totally sure where in the pipeline is best for font fetching via gulp. I put it in after SCSS but before JavaScript simply because fonts are imported using CSS, so it seemed logical.
  3. I'm also not totally sure if including the _fonts.scss file near the top of app.scss is the best option. I guess this would depend on how foundation-icon-fonts tends to be included based on tutorials and what not from the ZURB team.

@geotrev
Copy link
Author

geotrev commented Feb 19, 2018

I thought about it over night and decided it probably makes no sense to even have a _fonts.scss file for folks to throw their @font-face declarations into. So now it's simply the gulp addition.

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

Hi @geotrev. Thanks for the PR, this is a good idea. I made some remarks.

@@ -29,7 +29,7 @@ function loadConfig() {

// Build the "dist" folder by running all of the below tasks
gulp.task('build',
gulp.series(clean, gulp.parallel(pages, sass, javascript, images, copy), styleGuide));
gulp.series(clean, gulp.parallel(pages, sass, fonts, javascript, images, copy), styleGuide));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not totally sure where in the pipeline is best for font fetching via gulp

Since this is an asset, you can put it between javascript and images, or after images ;)

@@ -95,6 +95,10 @@ function sass() {
.pipe(browser.reload({ stream: true }));
}

function fonts() {
return gulp.src('./bower_components/foundation-icon-fonts/**/*.{ttf,woff,woff2,eot,svg}').pipe(gulp.dest('dist/assets/fonts'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hard-coding ./bower_components/foundation-icon-fonts in the Gulpfile, you can add the path in config.yml (as PATHS.fonts), but with the {ttf,woff,woff2,eot,svg} filter staying in the gulpfile.

Also, don't forget src/assets/fonts ;)

@@ -153,6 +157,7 @@ function watch() {
gulp.watch('src/pages/**/*.html').on('all', gulp.series(pages, browser.reload));
gulp.watch('src/{layouts,partials}/**/*.html').on('all', gulp.series(resetPages, pages, browser.reload));
gulp.watch('src/assets/scss/**/*.scss').on('all', sass);
gulp.watch(['src/assets/scss/**/*{tff,woff,woff2,eot,svg}.']['fonts']);
Copy link
Contributor

Choose a reason for hiding this comment

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

You're watching scss there, but same remark as above: rely on PATH.fonts.

Copy link
Author

Choose a reason for hiding this comment

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

Can you elaborate a bit on what you mean by PATH.fonts?

Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Just saw your other comment. It was hidden thanks to a commit I made just a few minutes ago.

@geotrev
Copy link
Author

geotrev commented Feb 19, 2018

@ncoden Thanks for the comments. I made those changes. One question I had though around these lines in config.yml:

  # Paths to static assets that aren't images, CSS, or JavaScript
  assets:
    - "src/assets/**/*"
    - "!src/assets/{img,js,scss}{,/**/*}"

Should fonts be added?

EDIT: I see now, it's being mentioned here:

// Copy files out of the assets folder
// This task skips over the "img", "js", and "scss" folders, which are parsed separately
function copy() {
  return gulp.src(PATHS.assets)
    .pipe(gulp.dest(PATHS.dist + '/assets'));
}

So in this case, it probably makes sense to add in fonts since it has its own parser. Thoughts? :)

@ncoden
Copy link
Contributor

ncoden commented Feb 19, 2018

Oups. I forgot that.
See the copy task: it copies everything in assets to dist/assets... like the font task you just added does. So unless we have others processing to do on fonts, we can forget the fonts task and simply rely on a line for fonts in config.yml

I'm sorry about that. I should have review the PR more carefully.

@geotrev
Copy link
Author

geotrev commented Feb 19, 2018

No worries! 👍

Didn't realize it was actually outputting those fonts. I think we should be fine not needing to make any changes actually, since the output appears to be identical. Thanks for taking the time to look this over!

@geotrev geotrev closed this Feb 19, 2018
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.

2 participants