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

Support a "default" key for object in scales #951

Merged
merged 20 commits into from
Dec 14, 2020
Merged

Support a "default" key for object in scales #951

merged 20 commits into from
Dec 14, 2020

Conversation

folz
Copy link
Contributor

@folz folz commented May 22, 2020

The implementation is straightforward, but I wanted to discuss something that I think is a pretty large change to the meaning of get and/or theme paths.

Before this PR, theme-ui does not enforce that get on a theme path will return a primitive value suitable for inclusion in CSS. It assumes either that the theme author will write the theme such that all paths lead to primitive values (that is, avoid using objects or arrays at the end of a path), or that the end programmer will ensure all paths will point to a primitive value (that is, 'red.500', never 'red').

This PR changes the above by moving some of the "ensure path points to a primitive" responsibility to theme-ui, by establishing the convention that path => object ? object.default. This is a special case, though. Why not path => object ? object.500, or path => object ? object.core? Should we support this for object-like values such as Map? What about other ways to store values: path => array ? array['default']? path => symbol ? symbol.description?

theme-ui also allows for sx to contain functions: sx={{color: (theme: Theme) => theme.colors.blue.500}}

This is less ergonomic but more precise than the object.default convention. It's also more typesafe, thanks to the Theme type. What if we allowed the theme to also contain themeGet functions? path => function ? function(theme). This would allow things like

colors: {
  default: themeGet('colors.500'),
  500: 'color500',
  ...
}

Is that a reasonable extension of the "ensure path points to a primitive" responsibility?

Anyway, we don't need to answer literally every question above. Maybe the single special case of object => default is fine here. But I still wanted to bring up the possible implications to have a discussion about.

Closes #936

@jxnblk
Copy link
Member

jxnblk commented May 26, 2020

Thanks for tackling this! I think this is a great addition to the API as-is -- it also sets up a nice convention for handling default variants.

What if we allowed the theme to also contain themeGet functions?

I think this is something we'd like to solve for. In the v1 issue, I put this as Idea: Provide some solution for self-referencing scales in other scale definitions. I don't think we need to solve for that in this PR, but feel free to take a stab at proposing what that could look like, if you have time

As far as the ensure path points to a primitive point, that's something I hadn't really considered, but might be good to at least warn users if an object or invalid value is being passed on as CSS.

packages/css/test/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@jxnblk jxnblk left a comment

Choose a reason for hiding this comment

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

Left a few nits/comments, but nothing blocking -- this looks great, thanks!

- Update color scale to use actual colors
- Include a test for variant object default behavior
- Leave comment indicating object values can't become valid CSS
@folz folz changed the title [RFC] "default" key for object in scales Support a "default" key for object in scales May 26, 2020
@folz
Copy link
Contributor Author

folz commented May 26, 2020

Thanks for the review! I made the changes you suggested. I'll open a new issue/PR about themeGet functions in themes.

@folz
Copy link
Contributor Author

folz commented May 26, 2020

Opened an issue to track themeGet reference support here: #961

@jxnblk
Copy link
Member

jxnblk commented May 27, 2020

Sweet! Thanks, I think this looks good to go, but want to let it sit for a bit so others have time to review or comment

@hasparus
Copy link
Member

hasparus commented Dec 1, 2020

This is a really good PR. Why didn't we merge it?
I'll handle the conflicts.

@lachlanjc lachlanjc added the enhancement New feature or request label Dec 2, 2020
hasparus and others added 14 commits December 9, 2020 18:46
…on/react-11.1.2

chore(deps): bump @emotion/react from 11.1.1 to 11.1.2
Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.7. **This update includes a security fix.**
- [Release notes](https://github.com/isaacs/ini/releases)
- [Commits](npm/ini@v1.3.5...v1.3.7)

Signed-off-by: dependabot-preview[bot] <[email protected]>
…staged-10.5.3

chore(deps-dev): bump lint-staged from 10.5.1 to 10.5.3
….3.7

chore(deps): [security] bump ini from 1.3.5 to 1.3.7
…-preset-gatsby-0.8.0

chore(deps-dev): bump babel-preset-gatsby from 0.6.0 to 0.8.0
…-jest-26.6.3

chore(deps-dev): bump babel-jest from 26.1.0 to 26.6.3
…-5.0.0

chore(deps-dev): bump execa from 4.1.0 to 5.0.0
@folz folz requested a review from hasparus as a code owner December 14, 2020 08:51
@vercel
Copy link

vercel bot commented Dec 14, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/cr5r1fra4
✅ Preview: https://theme-ui-git-folz-default-object.systemui.vercel.app

@hasparus
Copy link
Member

@folz, I'm gonna change this to __default to ensure people are using it knowingly. It turned out to be a bit surprising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: "default" key for objects in scales
4 participants