-
Notifications
You must be signed in to change notification settings - Fork 40
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
データのurl変更に対応するために、gen-fontsのスクリプトを修正 #4158
Conversation
Preview (prod) → https://4158-prod.traq-preview.trapti.tech/ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4158 +/- ##
=======================================
Coverage 86.35% 86.35%
=======================================
Files 66 66
Lines 4719 4719
Branches 564 564
=======================================
Hits 4075 4075
Misses 638 638
Partials 6 6 ☔ View full report in Codecov by Sentry. |
dda41e3
to
cc2411b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
フォントが生成されていること、新しいURLのフォントが適用されていることを確認しました
上のレビューで指摘されていること以外は良さそうです!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
まだ動作確認はしてないですが https://fonts.googleapis.com/css2?family=M+PLUS+1p:wght@400;700&display=swap の中だけ見ました
src: url(https://fonts.gstatic.com/s/mplus1p/v28/e3tmeuShHdiFyPFzBRrQRBEgfivGoOYmg_dUa_BuiDU9F33s7CtHVU4.119.woff2) format('woff2');
src: url(https://fonts.gstatic.com/s/mplus1p/v28/e3tmeuShHdiFyPFzBRrQRBEQcUnXkvc.woff2) format('woff2');
の両方に対応したくてユニークであればいいなら/\/v\d+\/(.+)\.woff2$
とか${family}.${weight}.${url.path}
で十分じゃないかなって思いました
できるだけフォント名短くしたいとかなら沿わないけど
pathだと/
が入ってだめか
↑確かにそれで十分そうなので前者の方針でやってみました |
生成されたの見るとファイル名長くてちょっと無駄な気もするけど、ロジック簡単になる方がよさそうなのでこれでよさそうですね |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
結果として正規表現直すだけになったのでdiffがやや冗長だけど動作自体は問題ないのでdiff減らすのめんどくさかったらマージしちゃってください:+1:
これ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コメントしましたが意図してたらそのままマージしちゃってください
const family = font['font-family'].replace(/[' ]/g, '') | ||
const weight = font['font-weight'].replace(/[' ]/g, '') | ||
return `${family}.${weight}.${i[1]}.woff2` | ||
|
||
const fontSrcWithId = getUrlFromSrc(font.src).match(/\/v\d+\/([^/]+)\.woff2/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/v28/hoge/e3tmeuShHdiFyPFzBRrQRBEQcUnXkvc.woff2
みたいなパスにはマッチさせないのは想定してますか?
末尾だけマッチさせるならこうかな
const fontSrcWithId = getUrlFromSrc(font.src).match(/\/v\d+\/([^/]+)\.woff2/) | |
const fontSrcWithId = getUrlFromSrc(font.src).match(/\/v\d+\/(?:[^/]*/)*([^/]+)\.woff2/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そうですね、今のところそれは考えなくていいかなと思っています
またエラー吐くようになったら変えるくらいでいいと思います
npm run gen-fonts
が失敗するようになっていたため、対応確認事項としては、
npm run gen-fonts
実行時にpublic/fonts
内にフォントが生成されていることと、プレビュー環境でフォントがちゃんと反映されていることが確認できれば多分大丈夫です