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

CSS Logical Properties should be theme-aware #794

Closed
folz opened this issue Mar 23, 2020 · 3 comments · Fixed by #797
Closed

CSS Logical Properties should be theme-aware #794

folz opened this issue Mar 23, 2020 · 3 comments · Fixed by #797
Labels
help wanted Extra attention is needed

Comments

@folz
Copy link
Contributor

folz commented Mar 23, 2020

Describe the bug

We use CSS Logical Properties to help support RTL and vertical languages. They are not theme-aware, so it's cumbersome to use theme values with them. This indirectly encourages developers to use physical direction instead, which makes it harder to support different writing modes.

For example, our styles may use marginInlineStart instead of marginLeft, which would be the same in a LTR language like English, but would instead mean marginRight when the language reads right-to-left like Hebrew.

To Reproduce
Use sx like so:

<Box sx={{
  marginInlineStart: 'medium'
}} />

Expected behavior

margin-inline-start: '8px'; /* or whatever `theme.space.medium maps` to */

Actual behavior

margin-inline-start: 'medium'; /* the literal string 'medium' */

Additional context

There are a lot of logical properties. I'm happy to do the work of adding them to theme-ui if you feel this is appropriate to include.

@jxnblk
Copy link
Member

jxnblk commented Mar 24, 2020

Thanks! We'd accept a PR to support this.

The dictionary for mapping theme scales to properties is here: https://github.com/system-ui/theme-ui/blob/master/packages/css/src/index.ts#L55

@jxnblk jxnblk added the help wanted Extra attention is needed label Mar 24, 2020
@folz
Copy link
Contributor Author

folz commented Mar 24, 2020

Sweet, I'll look to have that PR over in a day or so.

@folz
Copy link
Contributor Author

folz commented Mar 25, 2020

Update: the linked PR #797 looks to do the trick too

@jxnblk jxnblk mentioned this issue Apr 7, 2020
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants