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

fix: font-stretch doesn't match the regex #56

Merged
merged 1 commit into from
Sep 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/node_modules/
example*/out/
example*/node_modules
.*.swp
.*.swo
package-lock.json
1 change: 1 addition & 0 deletions example5/fonts.list
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Open+Sans:300&subset=cyrillic,cyrillic-ext,greek,greek-ext,latin-ext
13 changes: 13 additions & 0 deletions example5/gulpfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var gulp = require('gulp');
var googleWebFonts = require('../');

var options = { };

gulp.task('fonts', function () {
return gulp.src('./fonts.list')
.pipe(googleWebFonts(options))
.pipe(gulp.dest('out/fonts'))
;
});

gulp.task('default', ['fonts']);
11 changes: 11 additions & 0 deletions example5/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "example5",
"version": "0.0.1",
"description": "",
"main": "gulpfile.js",
"scripts": {
"test": "gulp"
},
"author": "Tingyu Huang",
"license": "ISC"
}
23 changes: 15 additions & 8 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,17 +236,22 @@ function getter(options) {
// since Google doesn't put there '.svg' suffix for some reason
// (but does that for other extensions).
var re = new RegExp([
"\\s*font-family:\\s*'([^']+)';",
"\\s*font-style:\\s*(\\w+);",
"\\s*font-weight:\\s*(\\w+);",
"\\s*src:([^;]*);",
".*(?:unicode-range:([^;]+);)?",
"\\s*font-family:\\s*'(?<family>[^']+)';",
"\\s*font-style:\\s*(?<style>\\w+);",
"\\s*font-weight:\\s*(?<weight>\\w+);",
".*(?:\\s*font-stretch:\\s(?<stretch>[^;]+);)?",
Copy link

Choose a reason for hiding this comment

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

Is the .* really needed?
It would actually be better if this entire "we match everything at once" would go away and we would match the stuff we really need instead.

Copy link
Owner

Choose a reason for hiding this comment

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

A proper parser CSS parser might be useful instead of using Regexes here. I'm open to PRs.

Copy link
Contributor Author

@TingyuHuang TingyuHuang Sep 30, 2021

Choose a reason for hiding this comment

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

The .* is cpoied from the pattern of unicode-range. It could help when there are lines between font-weight and font-stretch. I agree that a proper CSS parser would be helpful. This PR is a quick fix.

"\\s*src:(?<src>[^;]*);",
".*(?:unicode-range:(?<range>[^;]+);)?",
].join(''), 'm');

return formatData.apply(null, block.match(re, 'm'));

let found = block.match(re, 'm');
if (found === null) {
throw new Error("faild to match block");
Copy link

Choose a reason for hiding this comment

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

Typo, "faild" => "failed"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. It's fixed in #57 .

}
return formatData(found.groups);

function formatData(block, family, style, weight, src, range) {
function formatData(data) {
const { family, style, weight, stretch, src, range } = data;
var name = [family, style, weight].join('-') + '.' + ext;
var url = src.match(/url\(([^)]+)\)/)[1];
var locals = src.match(/local\([^)]+\)/g) || [];
Expand All @@ -257,6 +262,7 @@ function getter(options) {
name: name.replace(/\s/g, '_'),
url: url,
locals: locals,
stretch: stretch || 'normal',
range: range || 'U+0-10FFFF'
};
}
Expand All @@ -280,6 +286,7 @@ function getter(options) {
' font-style: $style;',
' font-weight: $weight;',
' font-display: ' + options.fontDisplayType + ';',
' font-stretch: $stretch;',
' src: $src' + format + ';',
' unicode-range: $range;',
'}'
Expand Down