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 styled-as-a-hoc #108

Open
sylvain-hamel opened this issue Nov 29, 2017 · 14 comments
Open

Add support for styled-as-a-hoc #108

sylvain-hamel opened this issue Nov 29, 2017 · 14 comments

Comments

@sylvain-hamel
Copy link

We have a HOC that calls to styled. We use that HOC a lot and because our components don't use styled directly, it seems like the plug-in is not able to generate class names based on the component name.

This code breaks the plug-in:

import * as styled1  from 'styled-components';
const styled = styled1.default;
const MySuperComponent = styled(({children, className}) =>
    (<div className={className}>{children}</div>))`color: red`;

I suppose that our HOC somehow creates the same conditions that the plug-in does not support.

Is this fixable? It there anything we can do in our HOC to give the plug-in it a hint?

@mxstbr
Copy link
Member

mxstbr commented Nov 30, 2017

Hmm, I think we could support this. Mind adding a failing test case based on the other tests? That'd be super helpful, thanks!

@mxstbr mxstbr changed the title plugin is unable to generate debugging class name Add support for styled-as-a-hoc Nov 30, 2017
@kitten
Copy link
Member

kitten commented Nov 30, 2017

I think we can just target styled, css, injectGlobal, and keyframes regardless of where those variables are defined... Because otherwise the detection code would get ridiculous.

I think that'd be a fair assumption to make. Wdyt, @mxstbr?

Edit: @sylvain-hamel btw HOCs for styling are not best practice, and there's some more caveats that I can think of that could hinder the plugin from working (now or in the near future). Can you post some small code snippet of that entire pattern, please, if that's possible that is? 😄

@mxstbr
Copy link
Member

mxstbr commented Nov 30, 2017

Because otherwise the detection code would get ridiculous.

I don't understand, what does the detection have to do anything? We already check that there's an import from styled-components and then we check if there's a styled() function call? It should only be a matter of adding .withConfig at the end of the styled function call no matter what's inside it, I don't think this has anything to do with detection.

@kitten
Copy link
Member

kitten commented Nov 30, 2017

@mxstbr I mean, code like @sylvain-hamel's breaks because he's presumably importing a local file instead of styled-components. So maybe we should get rid of the import/require-based detection.

@mxstbr
Copy link
Member

mxstbr commented Nov 30, 2017

Sorry? He's importing from styled-components though? I don't 100% understand what you're saying.

@mxstbr
Copy link
Member

mxstbr commented Nov 30, 2017

Oh you mean that this breaks the detection?

import * as styled1  from 'styled-components';
const styled = styled1.default;

Possibly. @sylvain-hamel can you try making that a normal import styled from 'styled-components' import?

@sylvain-hamel
Copy link
Author

Hi all, I'll send a test case shortly.

@sylvain-hamel
Copy link
Author

Are tests supposed to run on Windows? They passed on my Mac Book but 17 of them fail on my windows computer.

@sylvain-hamel
Copy link
Author

Ok, so here is the code.

This is the HOC:

// turns `styled` into a composable hoc
const composeStyledComponent = (value, ...rest) => 
    BaseComponent => styled(BaseComponent)(value, ...rest);

Code that uses the HOC:

const enhance = composeStyledComponent`color:red`;
const Component = ({className}) => <div className={className}>hello</div>;
const EnhancedComponent = enhance(Component);

And then some JSX somewhere

<EnhancedComponent/>

@sylvain-hamel
Copy link
Author

I'd like an update on this issue if possible. Is the code I provided clear? Can the plug-in be modified to support that use case? Thanks

@alehatsman
Copy link

I faced the same problem

import styled from 'styled-component'

const withStyled = (component) => styled(component)``

And then when I use that hoc in some other files, I always get the same class name withStyled and some hash.

@quantizor
Copy link
Collaborator

quantizor commented Sep 23, 2018 via email

@alehatsman
Copy link

alehatsman commented Sep 23, 2018

@probablyup yeah I understood that. Any ideas how to fix that? Quite ofther i want to be able to use style interpolation like

// button.js
import styled from 'styled-components'
import withStyled from '...'
import Icon from '...'

const StyledButton = styled.button`
// ....
`

const Button = ({ text, ...props }) => (
  <StyledButton {...props}>
    <Icon />
    { text }
  </StyledButton>
)

// and here is what i want to do, because later probably 
// i would need to add styles related to positioning. 
export default withStyled(Button)
import styled from 'styled-components'
import Button from 'components/Button'

const StyledCard = styled.div`
  display: flex
  ${Button} {
    margin-top: 10px;
  }
`

const Card = () => (
  <StyledCard>
   <Button />
  </StyledCard>
)

So what i see here is quite similar to all macro problems, now that thing doesn't compose, and the only way is to pass styles as arguments, or to create a wrapper, quite sad.

Any ideas?

@tabrindle
Copy link

Running into what I think is the same issue, but there's another distressing element to it - it works on macOS, and fails on Linux (CI).

import styled from 'styled-components';

export const styledSomething = Component => styled(Component)`
  display: flex;
`;

I didn't write any of this HoC code, and to be honest it seems unnecessary here, but why would it work on one platform and not the other? I'm curious about this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants