-
Notifications
You must be signed in to change notification settings - Fork 296
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
Pixelated MS Sans Serif #27
Conversation
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.
Got a Q inline about network requests with this! Let me know if my comment makes any sense.
(Thanks for this, though, I like it)
build.js
Outdated
@@ -45,6 +53,7 @@ function buildCSS() { | |||
.use(require("postcss-inline-svg")) | |||
.use(require("postcss-css-variables")({ preserve: "computed" })) | |||
.use(require("postcss-calc")) | |||
.use(require("postcss-copy")({ dest: "build" })) |
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.
Could postcss-url
help us here, with { url: 'inline' }
? https://github.com/postcss/postcss-url
Otherwise folks would have to do quite a few network requests to get the font, right? And less portable
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.
Inlining addressed, thanks :) #27 (comment)
Can we maintain the original filenames here with the template
option?
The fonts are 7kb per weight for the woff2 caniuse 94.9% and 9kb for the woff caniuse 97.5%. Pros to inlining:
Cons to inlining:
|
I hear you on the body text - definitely a tradeoff when it comes to historical accuracy vs aesthetics/ergonomics :). My most recent commit has a blacklist. Feel free to fixup the inlining stuff or anything else, and thanks for making such a cool library. |
Will this font handle non-latin characters well? |
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.
oh god what is this i just wanted to leave a code comment
@kaytwo thanks for all your hard work on this! Wanna address the latest comments re: denylist? |
@kaytwo wanna rebase your branch? trying to review but there's some old stuff in there |
I'm in rebase hell atm, I will get it taken care of, but if you want to see WIP it's in kaytwo@cdb6a5a |
Changes since we last spoke:
|
Cool :) Sorry for rebase hell! Lemme dive in and fix these up for ya (I did a clean sweep of all my windows line-endings) |
Here's a diff https://github.com/jdan/98.css/compare/fixed-line-endings that retains your commit attribution. Lemme see if I can force push to you to keep this PR around... |
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.
lgtm
looks so good
amaze
8fb783a is the commit that lands the substantive changes; the fully built docs/package are in the other commit.
Features: