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 support for CSS Logical Properties #797

Merged
merged 4 commits into from
May 7, 2020
Merged

Conversation

lachlanjc
Copy link
Member

Pretty sure I got them all :)

Closes #794

Copy link
Contributor

@folz folz left a comment

Choose a reason for hiding this comment

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

LGTM! One comment below. I'm also not a maintainer so feel free to disregard.

@@ -160,6 +160,25 @@ test('handles all core styled system props', () => {
})
})

test('handles css logical properties', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit nitpicky, but I think it'd be worthwhile to include all the logical properties in a test since any one of them could regress.

@peduarte
Copy link
Contributor

Nice, good to see this 😍

Just today I created this issue in Styled System, since I use @styled-system/css instead of Theme UI's.

👉 styled-system/styled-system#1268

How do you feel about adding aliases to the margin and padding logical properties? I think since we have aliases for margin and padding it could make sense here too.

  • mbs: marginBlockStart
  • mbe: marginBlockEnd
  • mis: marginInlineStart
  • mie: marginInlineEnd
  • pbs: paddingBlockStart
  • pbe: paddingBlockEnd
  • pis: paddingInlineStart
  • pie: paddingInlineEnd

@lachlanjc
Copy link
Member Author

@jxnblk Thoughts on this? Should we add aliases for spacial logical properties?

@folz
Copy link
Contributor

folz commented Apr 15, 2020

A point of confusion I could see is that marginBlock is a valid rule, but marginBottom has already taken the mb abbreviation. mbk maybe?

marginInline could also be abbreviated mi if we go forward with logical abbreviations.

@jxnblk
Copy link
Member

jxnblk commented Apr 21, 2020

I don't think we should worry about shorthands for this PR. @peduarte note that @styled-system/css is deprecated in favor of @theme-ui/css so this feature probably won't land in the old package

@peduarte
Copy link
Contributor

Ah, I had a feeling that this would be the case 🙂

SGTM 👍

@lachlanjc
Copy link
Member Author

@jxnblk Are you looking for any changes here or is this good to merge?

@jxnblk
Copy link
Member

jxnblk commented May 7, 2020

Sorry for the delay! This looks great, thanks for getting this together!

@jxnblk jxnblk merged commit d47fe6b into system-ui:master May 7, 2020
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.

CSS Logical Properties should be theme-aware
4 participants