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

Add Modern Font Stacks #498

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add Modern Font Stacks #498

wants to merge 15 commits into from

Conversation

Jothsa
Copy link
Contributor

@Jothsa Jothsa commented May 1, 2024

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!

Copy link

stackblitz bot commented May 1, 2024

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

@argyleink
Copy link
Owner

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.

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

overall the PR looks great! i've got a couple open questions to consider:

  1. are these fonts better as an optional import rather than being part of the default bundle?
  2. is the typography documentation section overwhelming now with all the typography example blocks? we could consider using the <details> element OR make a switcher that lets people choose a font prop from a dropdown, which updates an example or all the typography examples
  3. how can we give @danklammer and Modern Font Stacks more cred for the values?

i'll pose it to the group in discord and see how folks feel about it

thanks for this, i'm excited about it 🙂

docsite/index.html Outdated Show resolved Hide resolved
docsite/index.html Outdated Show resolved Hide resolved
src/props.fonts.css Outdated Show resolved Hide resolved
src/props.fonts.js Outdated Show resolved Hide resolved
@Jothsa
Copy link
Contributor Author

Jothsa commented May 3, 2024

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?

docsite/index.html Outdated Show resolved Hide resolved
docsite/index.html Outdated Show resolved Hide resolved
docsite/index.html Outdated Show resolved Hide resolved
@Jothsa
Copy link
Contributor Author

Jothsa commented May 8, 2024

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

@argyleink
Copy link
Owner

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
Copy link
Contributor Author

Jothsa commented Jun 4, 2024

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
Copy link
Contributor Author

Jothsa commented Jun 5, 2024

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
Copy link
Contributor Author

Jothsa commented Jun 5, 2024

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
Copy link
Owner

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 👍🏻

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

looks great! i made 1 little suggestion which i think i can just inject/accept and this is g2g. appreciate the work!

src/extra/normalize.src.css Outdated Show resolved Hide resolved
Comment on lines -2 to -4
--font-sans: system-ui,-apple-system,Segoe UI,Roboto,Ubuntu,Cantarell,Noto Sans,sans-serif;
--font-serif: ui-serif,serif;
--font-mono: Dank Mono,Operator Mono,Inconsolata,Fira Mono,ui-monospace,SF Mono,Monaco,Droid Sans Mono,Source Code Pro,monospace;
Copy link
Owner

Choose a reason for hiding this comment

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

actually, 1 more comment, as changing these names means i need to make a breaking change release. alternatively, we could keep these same names or just keep the original props, that way it's a minor version bump and no one's apps break.

which is better?

  1. keep the previous props in additional to the new ones
  2. create vars that map the old names to the most relevant new ones
  3. keep the previous props and make the new ones an additional/optional import

i'll toss this into the discord too, see what folks think!

Copy link

Choose a reason for hiding this comment

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

I think option 1 or 2 is best. I kinda prefer 2 but I appreciate the hesitation to have breaking changes so I'd not complain at either. In my opinion these are small enough that I don't think we need to hide them behind a flag, and I love the idea of having the prototyping power of these well thought out props easily available. Also most people who care about the css weight should probably be using jit.

Copy link
Contributor Author

@Jothsa Jothsa Jun 11, 2024

Choose a reason for hiding this comment

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

My vote is to map the old props to the new ones and remove them in the next major version. Maybe add a deprecation notice to the doc site?

@mobalti
Copy link
Contributor

mobalti commented Jun 12, 2024

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?

@SanderElias
Copy link

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:

  • map the old props to the new
  • add all into the main import
  • add deprecation notice in docs (as needed?)
  • move everything into its self-contained import in the next version.

@Jothsa
Copy link
Contributor Author

Jothsa commented Jun 19, 2024

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?

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.

None yet

6 participants