argyleink / open-props

CSS custom properties to help accelerate adaptive and consistent design.
https://open-props.style
MIT License
4.52k stars 184 forks source link

Add Modern Font Stacks #498

Closed Jothsa closed 1 day ago

Jothsa commented 2 months ago

Let me know if there's anything else I need to do. I think it might be nice to have an explanation of the benefits of using these props over an external font, but I'm not sure where it would fit. Thanks!

stackblitz[bot] commented 2 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

argyleink commented 2 months ago

Look at all that tucked into nice little props 👍🏻
I do wonder if we need to break these modern font stacks into their own import?

Screenshot 2024-05-02 at 2 31 26 PM

Screenshot 2024-05-02 at 2 32 01 PM Screenshot 2024-05-02 at 2 32 08 PM

wonder if we should put ^ these behind a summary details so they don't take up so much space? was cool when there were a few, but now there's a lot.

Jothsa commented 2 months ago

Good point about giving them more credit. I think it would also be nice to have some information explaining why people would want to use these over an external font. Maybe that explanation and better credit could be in a paragraph after the "Font Families" heading?

Jothsa commented 1 month ago

I've got finals this week and next week so I won't be able to do much here until after that

argyleink commented 4 weeks ago

I've got finals this week and next week so I won't be able to do much here until after that

hope finals went well! let us know if you need help closing this out. it's a sweet addition.

Jothsa commented 4 weeks ago

They did (still waiting for some results though 😶)! Thanks! So sorry for the delay— life’s been coming at me and my ADHD has had me hyperfixating on another project (built with open-props) lol. I’ll get back to this pr soon!

Jothsa commented 4 weeks ago

I think these fonts would work best as a separate import. However, I think that the font-weights, sizes, etc should remain in the default bundle. Do you think it would be confusing to have the font-families separate from the other font props? Should the font family props be in a separate import, should all the font related props be in a separate import, or should they remain in the main bundle?

Jothsa commented 3 weeks ago

Alright, I think that's everything except if you want them split into a separate import. Do you want to keep --font-system-ui? It's from modern font stacks, but I think it's unnecessary. It is set to system-ui, sans-serif

argyleink commented 3 weeks ago

Alright, I think that's everything except if you want them split into a separate import. Do you want to keep --font-system-ui? It's from modern font stacks, but I think it's unnecessary. It is set to system-ui, sans-serif

rad ty! i'm just returning from conferencing and ready to review this. I do think it's a good idea to include that prop as it's often missed by folks and ChromeOS for example doesn't yet support system-ui while it does sans-serif. healthy fallback habit. Thanks for asking!

I'll give this a review now 👍🏻

mobalti commented 3 weeks ago

I think we should consider the overall approach for introducing new props in the library:

SanderElias commented 2 weeks ago

I think we should consider the overall approach for introducing new props in the library:

  • Should we add them to the main import, as we did with Static Sizes and Drawn Borders? (While they are great, I'm still not convinced they should be in the main bundle.)
  • Or should we keep everything as separate imports from now on?

When the added props are entirely new for the current release, they should be imported separately. I fully agree with this. However, as those in this PR are touching existing stuff, it must be in the main. Otherwise, it would be breaking. And then, as proposed, fix this in the next version. I suggest proceeding with the current plan. For completeness, that is:

Jothsa commented 2 weeks ago

I've added the old values and mapped them to the new ones. Will postcss-jit-props be able to handle that correctly? Do you think a deprecation notice is needed?

Jothsa commented 1 day ago

The variables should be good to go. I have 2 quick questions. Do you think we need a deprecation notice for the old values? Do you think we should note that --monospace-code is different in open-props than in modern font stacks? I think it's ok to quietly remove the old font values. I also think it's ok to not add the note, but thought I'd run it by you. Thanks for all the help!

argyleink commented 1 day ago

Do you think we need a deprecation notice for the old values?

I've made a new issue to hold this task, I'll try and make sure it's part of OPv2 👍🏻


Do you think we should note that --monospace-code is different in open-props than in modern font stacks?

Seems like a fair small note to add to the docs. Use a blockquote like this? Screenshot 2024-07-02 at 9 30 58 AM

Jothsa commented 1 day ago

Done

Jothsa commented 1 day ago

No problem! Thanks for your support and time!