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

Pixelated MS Sans Serif #27

Merged
merged 9 commits into from
Apr 23, 2020
Merged

Pixelated MS Sans Serif #27

merged 9 commits into from
Apr 23, 2020

Conversation

kaytwo
Copy link
Contributor

@kaytwo kaytwo commented Apr 22, 2020

8fb783a is the commit that lands the substantive changes; the fully built docs/package are in the other commit.

Features:

  • falls back to Arial if you don't upload the pixelated fonts
  • updates the docs and the build to match new features
  • looks hot

Copy link
Owner

@jdan jdan left a 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" }))
Copy link
Owner

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

Copy link
Owner

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?

@kaytwo
Copy link
Contributor Author

kaytwo commented Apr 22, 2020

The fonts are 7kb per weight for the woff2 caniuse 94.9% and 9kb for the woff caniuse 97.5%.

Pros to inlining:

  • Should definitely choose one or the other, inlining both would be dumb
  • 14kb (or 16kb) add'l text in the css file. This is compressed already so transfer encoding gzip won't make it any smaller.

Cons to inlining:

  • Browser will only request the 2 font files it needs based on compatibility, thus maximizing compat
  • If you're on h2 you're probably better off without inlining, you might be able to do some push magic, I don't know how to do/hint that from the css file though or whether you need to
  • If someone doesn't want to include the pixelated fonts, they will need to rebuild the package/css file.

@jdan
Copy link
Owner

jdan commented Apr 22, 2020

Cool, ty for the explanation.

I'll be honest - I think this looks really sharp for buttons and labels but I'm less sold when it comes to headings and paragraphs 😭

image

@kaytwo
Copy link
Contributor Author

kaytwo commented Apr 22, 2020

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.

@hedgehog-online
Copy link

Will this font handle non-latin characters well?

Copy link

@hedgehog-online hedgehog-online left a 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

style.css Outdated Show resolved Hide resolved
@jdan
Copy link
Owner

jdan commented Apr 23, 2020

@kaytwo thanks for all your hard work on this! Wanna address the latest comments re: denylist?

@jdan
Copy link
Owner

jdan commented Apr 23, 2020

@kaytwo wanna rebase your branch? trying to review but there's some old stuff in there

image

@kaytwo
Copy link
Contributor Author

kaytwo commented Apr 23, 2020

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

@kaytwo kaytwo requested a review from jdan April 23, 2020 16:40
@kaytwo
Copy link
Contributor Author

kaytwo commented Apr 23, 2020

Changes since we last spoke:

  • Whitelist for button, window, title-bar (the latter sometimes gets used in the docs outside of a window). I could imagine based on your aesthetic preference, things like fieldsets or other elements could also be included in the whitelist.
  • "Override" classes pixelated and vectorized which can be used to force a given element to use the different text rendering styles.
  • Changed * to body for the root styling because * is always more specific than my whitelist if it's on a child node.

@jdan
Copy link
Owner

jdan commented Apr 23, 2020

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)

@jdan
Copy link
Owner

jdan commented Apr 23, 2020

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...

@jdan
Copy link
Owner

jdan commented Apr 23, 2020

Goodness it looks so handsome
image

Copy link
Owner

@jdan jdan left a 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

@jdan jdan merged commit 5cc23ba into jdan:master Apr 23, 2020
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.

3 participants