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 sx prop to Themed components #1350

Closed
hoobdeebla opened this issue Dec 9, 2020 · 12 comments · Fixed by #2250
Closed

Add sx prop to Themed components #1350

hoobdeebla opened this issue Dec 9, 2020 · 12 comments · Fixed by #2250
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@hoobdeebla
Copy link
Contributor

As of v0.6, the Styled component was renamed to Themed. After renaming all imports and updating theme-ui, the sx styles on my Themed components are no longer applied unless I explicitly state @jsxImportSource theme-ui (auto runtime) or @jsx jsx (classic). The sx object is also no longer recognized by VS Code as an instance of ThemeUIStyleObject for Themed, whereas it is properly reflected for Box, Image, etc.

This seems to be a regression made somewhere between 0.5 and 0.6, as all my Styled components worked as intended up until the 0.6 release. Tested and confirmed on all 0.6 versions.

Example code:

import { Box, Flex, Image, Themed } from 'theme-ui'

export default () => (
  <Flex sx={{ width: 100vw, height: 100vh, justifyContent: 'center', alignItems: 'center' }}>
    <Box sx={{ flexGrow: 1 }}>
      <Image ... />
      <Themed.h1 sx={{ textAlign: 'center', textTransform: 'uppercase' }}>Enter Site</Themed.h1>
    </Box>
  </Flex>
)

Expected:
expected

Actual:
actual

@atanasster
Copy link
Collaborator

Hi @hoobdeebla - thanks for submitting this issue.

I don't think the sx prop worked without the jsx pragma before.

Here is a codesanbox with theme-ui 0.3.4 and using a component with the jsx pragma and one without

https://codesandbox.io/s/nameless-sun-hspem-forked-h2q3c

However, if you can re-create the issue I will be glad to take a look.

@hasparus any feedback about the ThemeUIStyleObject question?

@lachlanjc
Copy link
Member

Hmm. I feel like sx should work on Themed just like it does on our components, without the pragma

@atanasster
Copy link
Collaborator

@lachlanjc - it used to be that sx would work on some components that are just passing it down to ie <Box />, and that I believe is what is not working anymore since ~0.5 (now works only if you pass sx as part of a spread operator of props - @hasparus had a good reasoning why its like that).

For Styled/Themed, I dont think it ever worked since they are not using Box. In my opinion, all theme-ui components should work without a pragma by default. Maybe we can document using the babel config/preset as the first-choice option for using theme-ui and mostly get away from the pragma.

@hasparus hasparus self-assigned this Dec 12, 2020
@hasparus
Copy link
Member

@lachlanjc - it used to be that sx would work on some components that are just passing it down to ie , and that I believe is what is not working anymore since ~0.5 (now works only if you pass sx as part of a spread operator of props - @hasparus had a good reasoning why its like that).

It should be okay now. That was just a type issue. We had sx missing from Box types. Is it still broken?


@atanasster is right, Themed.X never supported sx prop on its own.

This is how it looks now:
image

And this is how it looked 10 months ago

export const themed = (key: keyof IntrinsicSxElements) => (props: ThemedProps) =>
  css(get(props.theme, `styles.${key}`))(props.theme) as Interpolation<ThemedProps>

@hoobdeebla
Copy link
Contributor Author

My mistake, I was testing going back and forth between classic/pragma and automatic/@jsxImportSource and must have gotten confused along the way. I went back and tested on a 0.3 site and confirmed that Themed.X never had a sx prop without using the pragma.

IMO the sx prop on Themed.X components should work without a pragma and without a custom Babel config, just as Box does now. Using the Babel config suggested in #1335 on a Gatsby site broke gatsby-plugin-mdx (I removed "importSource": "theme-ui" to fix this), so I have only added @jsxImportSource theme-ui to pages that necessitate use of the sx prop on non-Theme UI components. Currently that includes pages that use Themed.X - confusing, right? I avoid using the pragma most of the time by aliasing to Box, i.e. <Box as='article' sx={...}> or <Box as={GatsbyLink} sx={...}>, but doing <Box as='h1' variant='styles.h1' sx={...}> defeats the purpose of having <Themed.h1 sx={...}> in the first place, so I end up declaring the import source for files that use Themed.X. After Themed.X has a native sx prop, I won't need to declare @jsxImportSource theme-ui anywhere.

As an aside, it would be cool if Themed.X got the variant and style props like Box as well.

@lachlanjc lachlanjc changed the title [0.6] Themed component sx styles not applied Add sx prop to Themed components Dec 30, 2020
@lachlanjc lachlanjc added the enhancement New feature or request label Dec 30, 2020
@freeman-lab
Copy link

Hi! We're big fans and users of theme-ui over at carbonplan, and just wanted to +1 the behavior @hoobdeebla and @lachlanjc describe. Supporting extra sx props on Themed elements without the pragma would be really nice. A fairly common pattern for us is to want to use <Themed.x> components for consistency but in a few cases make minor tweaks, e.g. changing the margin on the first header of a page.

Similar to @hoobdeebla , our alternative now is to drop down to something like <Text as='h1' variant='styles.h1' sx={{my: [4]}}> instead of <Themed.h1 sx={{my: [4]}}>. Totally doable! But obviously the latter would be cleaner, and makes us less likely to forget to ensure semantic consistency.

@lachlanjc
Copy link
Member

@freeman-lab Yeah, I discovered your awesome repos the other day & added the links to our Resources docs! Love what you're working on there—I'm also using Theme UI for some climate-related work :)

@atanasster
Copy link
Collaborator

@hoobdeebla yes very valid points for a new feature request. I was just mentioning that it did not exist in previous versions.

@hasparus
Copy link
Member

It doesn't cost us much to support sx on Themed.X components. Let's do it.

After Themed.X has a native sx prop, I won't need to declare @jsxImportSource theme-ui anywhere.

However, I'm surprised seeing you all writing jsxImportSource. I was under impression that one of the main points of automatic runtime was to remove the need of using /** @jsx */ pragma — de facto a builtin @wordpress/babel-plugin-import-jsx-pragma / @emotion/babel-plugin-jsx-pragmatic. Is it because of the performance overhead of using jsx everywhere?

@fwextensions
Copy link

Is this working now? I thought it wasn't at first, and started searching for a fix. But I'd confused myself with some duplicated code, and the sx prop seems to be working on Themed components without adding the pragma.

@hasparus
Copy link
Member

hasparus commented Jun 2, 2022

Hey @fwextensions, I just doublechecked the code, and we do use Theme UI's jsx function to create Themed components, so sx should work. I think I missed it in the types though. Are you using TS or JS?

@fwextensions
Copy link

I'm using JS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants