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

Document theme properties' default values #2026

Closed
benface opened this issue Nov 29, 2021 · 8 comments · Fixed by #2087
Closed

Document theme properties' default values #2026

benface opened this issue Nov 29, 2021 · 8 comments · Fixed by #2087
Labels
documentation Changes only affect the documentation released This issue/pull request has been released.
Milestone

Comments

@benface
Copy link

benface commented Nov 29, 2021

Describe the bug
I noticed that if I omit some keys from the theme object I pass to the ThemeProvider, such as breakpoints and fontSizes, some default values will be used, and I couldn’t find that documented anywhere so I don’t think it’s by design. The default values for these two keys are the same as in the base preset, but I can confirm that it’s not the whole base preset, as fonts and fontWeights behave as expected, for example. Could it have something to do with the fact that breakpoints and fontSizes are arrays? What’s especially strange is that I can’t find the default values in the theme returned by the useTheme hook; they only pop up when I try to use them (e.g. fontSize: 1, or any value set to an array will flip at 40em, then 52em, etc.).

To Reproduce

  1. Leave out the breakpoints and fontSizes keys from your theme.
  2. Render <div sx={{ fontSize: 1 }} />
  3. Notice the rendered CSS is font-size: 14px.

Expected behavior
I would’ve expected the rendered CSS to be font-size: 1px, which is the case when the theme has a fontSizes key set to an empty array.

@lachlanjc lachlanjc added this to the v1.0 milestone Nov 30, 2021
@lachlanjc
Copy link
Member

This is on the list in #832! Relevant code here: https://github.com/system-ui/theme-ui/blob/develop/packages/css/src/index.ts#L76-L79

@benface
Copy link
Author

benface commented Nov 30, 2021

Ah ha, thank you @lachlanjc. I see “remove default fontSizes and space values”; should breakpoints be added to that list?

@lachlanjc
Copy link
Member

lachlanjc commented Dec 1, 2021

Hmm, I'm of two minds there. I think it makes sense for fontSize & space since those are visible. But having some default breakpoints doesn't seem so bad? Setting them up is a bit of a pain & there are real scenarios where you don't need to customize those. But could definitely be convinced otherwise.

@benface
Copy link
Author

benface commented Dec 1, 2021

You’re right. As long as it’s documented, having some default breakpoints makes sense!

@lachlanjc lachlanjc mentioned this issue Dec 29, 2021
16 tasks
@lachlanjc
Copy link
Member

lachlanjc commented Jan 17, 2022

I've changed my mind on this. I'm actually in favor of keeping these default values — I think a big part of the appeal of Tailwind & how easy it is to get started with Tailwind is their excellent defaults. While most users will continue to want to set their own theme, anything we can do to make getting started faster seems like a win. & as you said, it's still easy enough to reset these values to be empty. Will plan to make this clearer in documentation.

@lachlanjc lachlanjc added the documentation Changes only affect the documentation label Jan 17, 2022
@lachlanjc lachlanjc changed the title Some theme properties have undocumented default values Document theme properties' default values Jan 17, 2022
@benface
Copy link
Author

benface commented Jan 17, 2022

Cool, that works too!

@hasparus
Copy link
Member

🚀 Issue was released in v0.14.0 🚀

1 similar comment
@hasparus
Copy link
Member

🚀 Issue was released in v0.14.0 🚀

@hasparus hasparus added the released This issue/pull request has been released. label Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Changes only affect the documentation released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants