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

Use hooks #2342

Closed
4 tasks
mxstbr opened this issue Jan 21, 2019 · 27 comments
Closed
4 tasks

Use hooks #2342

mxstbr opened this issue Jan 21, 2019 · 27 comments

Comments

@mxstbr
Copy link
Member

mxstbr commented Jan 21, 2019

We should use React hooks once they land in a stable release! Here are a couple first ideas:

  • Rework theming system to be based on useContext to avoid DevTools bloat with ThemeConsumer components
  • Implement useTheme hook (see Implement 'useTheme' using Hooks API #2340)
    • Maybe remove withTheme HOC in favor of the hook?
  • Rework StyleSheet infrastructure to use hooks

Any other ideas for how we could leverage hooks?

@quantizor
Copy link
Contributor

Can you add this to the v5 rollup ticket?

@quantizor
Copy link
Contributor

maybe remove withTheme HOC in favor of the hook?

I'm 👎 on this because there are use cases where you need the theme in bound functions and that flow becomes awkward without dependency injection.

@TrySound
Copy link

Hooks work fine with flow.

@quantizor
Copy link
Contributor

quantizor commented Jan 21, 2019

We could probably introduce a useStyle hook that injects immediately once called and returns a className. And we could internally do a useEffect thing to automatically clean it up on unmount of the component in the stylesheet.

@quantizor
Copy link
Contributor

quantizor commented Jan 21, 2019

Something like:

const className = useStyle`
  color: ${p => p.theme.colors.red};
`;

Is that possible, using a TTL as a hook?

@TrySound
Copy link

I thought styled components are against classes. How are you gonna implement SSR with this?

@imbhargav5
Copy link
Member

@probablyup Exactly what I had in mind.

@TrySound
Copy link

Emotion introduced jsx pragma to solve SSR problem.

@quantizor
Copy link
Contributor

I thought styled components are against classes.

What do you mean? Like React classes? Ideally we'd like to use lighter representations in the library for things, but we have no overall objection to them.

@mxstbr
Copy link
Member Author

mxstbr commented Jan 21, 2019

We could probably introduce a useStyle hook that injects immediately once called and returns a className.

I don't really like this, that would fit better in a separate / complementary library based on the same (new) core StyleSheet infra to share code. Maybe styled-hook?

@quantizor
Copy link
Contributor

I guess so? Would be pretty easy to add and just another way to do things.

@mxstbr
Copy link
Member Author

mxstbr commented Jan 21, 2019

Yeah but it does not fit well into the "never see another class again"-thing that the entire styled-components API has right now.

I would much rather this (seemingly "lower-level") hook live in another package, especially once we land the monorepo refactor (#2326) and the new StyleSheet infra (which will also be a separate package) it should be easy.

@quantizor
Copy link
Contributor

I'd kind of rather it just be a different subfolder inside styled-components rather than a separate package. I am extremely against the emotion way of doing things where you have to consume 5-7 different packages.

@mxstbr
Copy link
Member Author

mxstbr commented Jan 21, 2019

I do not like that either at all, I think the new StyleSheet infra does need to be a separate package that's set as a dependency on styled-components though. That means users still only have to install one package (styled-components), but other libraries can potentially share the StyleSheet infra and build custom APIs that aren't in styled-components (like useStyles) on top!

Does that make sense?

@lstkz
Copy link

lstkz commented Mar 11, 2019

interface FooProps {
  title: string;
  description: string;
  bold: boolean;
}

function Foo(props: FooProps) {
  const { title, bold, description } = props;

  const theme = useTheme();

  const Wrapper = useStyled.div`
    padding: 10px;
  `;

  const Title = useStyled.h1`
    color: ${theme.primary};
    font-weight: ${bold ? 'bolder' : 'normal'};
    font-size: 18px;
  `;

  const Description = useStyled.div`
    font-weight: ${bold ? 'bold' : 'normal'};
    font-size: 12px;
  `;

  return (
    <Wrapper>
      <Title>{title}</Title>
      <Description>{description}</Description>
    </Wrapper>
  );
}

Such API would work for me. It happens very often than a component has many private components. It's overwhelming when using it with TypeScript, because you must add a type annotation to every child components. In the above example, only the parent component is annotated.
It could be implemented as an add-on to current functionality.

@kerwitz
Copy link

kerwitz commented Mar 22, 2019

In the meantime, here is an exemplary hooks implementation:

import styled, { css } from 'styled-components';

export const useStyled = (callback: ({ styled, css }) => JSX.Element): JSX.Element => {
    const Element = React.useRef(null);

    if (!Element.current) {
        Element.current = callback({ styled, css });
    }

    return Element.current;
};
import { useStyled } from './hooks';

const MyComponent = (props) => {
    const Container = useStyled(({ styled, css }) => styled.div`
        background: #000;

        ${props => props.inverse && css`
            background: #fff;
        `}
    `);

    return <Container inverse={true}>Hello world</Container>;
};

@lstkz
Copy link

lstkz commented Mar 23, 2019

Here is my version from the example above.
It's also a simple wrapper hook.

Purpose:

  • Avoid extra interface typing in TypeScript.
  • Only for private children components, that are styled based on the parent props.
  • For shared components, use a standard styled.div approach.
  • It can also be used as a replaced for this syntax:
// OLD WAY
interface OldProps {
  className?: string;
  children: string;
}

const OldApproach = styled((props: OldProps) => {
  // compute something here
  return <div className={props.className}>{props.children}</div>;
})`
  // styles here
`;

// NEW WAY
interface NewProps {
  children: string;
}

const NewApproach = (props: NewProps) => {
  // compute something here
  const Wrapper = useStyled.div(`
  // styles here
  `);
  return <Wrapper>{props.children}</Wrapper>;
};

Demo Syntax:

interface FooProps {
  title: string;
  description: string;
}

export function StyledSample(props: FooProps) {
  const { title, description } = props;
  const [bold, setBold] = React.useState(false);

  const Wrapper = useStyled.div(`
    padding: 10px;
    width: 200px;
    text-align: center;
  `);

  const Title = useStyled.h1(` 
    font-weight: ${bold ? 'bolder' : 'normal'};
    font-size: 20px;
  `);

  const Description = useStyled.p(`
    font-weight: ${bold ? 'bold' : 'normal'};
    font-size: 14px;
  `);

  // this should be a shared component
  const Button = useStyled.button(`
    color: white;
    border-radius: 3px;
    padding: 0.25em 1em;
    margin: 0.5em 1em;
    border-width: 2px;
    border-style: solid;
    border-color: palevioletred;
    border-image: initial;
    background: palevioletred;
  `);

  return (
    <Wrapper>
      <Title>{title}</Title>
      <Description>{description}</Description>
      <Button onClick={() => setBold(!bold)}>toggle bold</Button>
    </Wrapper>
  );
}

CodeSandbox https://codesandbox.io/s/3yl3wm6zp

@maxijonson
Copy link

@BetterCallSKY Hey, I like that way of using styled hooks! I tried taking your useStyled.tsx from the codesandbox, but some errors with Typescript come up.

Error at line 156 (ret: any, tag: keyof JSX.IntrinsicElements) => {

Argument of type '(ret: any, tag: "symbol" | "object" | "a" | "abbr" | "address" | "area" | "article" | "aside" | "audio" | "b" | "base" | "bdi" | "bdo" | "big" | "blockquote" | "body" | "br" | "button" | ... 153 more ... | "view") => any' is not assignable to parameter of type '(previousValue: any, currentValue: string, currentIndex: number, array: string[]) => any'.
  Types of parameters 'tag' and 'currentValue' are incompatible.
    Type 'string' is not assignable to type '"symbol" | "object" | "a" | "abbr" | "address" | "area" | "article" | "aside" | "audio" | "b" | "base" | "bdi" | "bdo" | "big" | "blockquote" | "body" | "br" | "button" | "canvas" | ... 152 more ... | "view"'.ts(2345)

Error at line 158 to 160 (styled[tag]
${(props: any) => props.css || ""}
``)

Cannot invoke an expression whose type lacks a call signature. Type 'ThemedStyledFunction<"a", any, {}, never> | ThemedStyledFunction<"abbr", any, {}, never> | ThemedStyledFunction<"address", any, {}, never> | ThemedStyledFunction<"area", any, {}, never> | ... 167 more ... | ThemedStyledFunction<...>' has no compatible call signatures.

Any idea how to fix those? Using SC v4.1.3

@lstkz
Copy link

lstkz commented Mar 27, 2019

Try to use a newer version of Typescript. microsoft/TypeScript#28795
or change tag: keyof JSX.IntrinsicElements to tag: any

@maxijonson
Copy link

maxijonson commented Mar 27, 2019

Did you mean older? Because I already tried with the latest one (3.3.4000). I tried going to Typescript 3..1.x since the original post suggests it worked before 3.2.x, but that didn't work... Also tried the codesandbox version of Typescript (3.3.3) which also didn't work. Tried changing to tag: any but then an error appeared on styled[tag]

Element implicitly has an 'any' type because type 'ThemedBaseStyledInterface<any>' has no index signature.

Could this be a compiler option? For now, I've added a @ts-ignore comment to suppress this error, but didn't have a chance to try it out yet, I'll edit this when I do.
EDIT:
Works like a charm, thanks! Additionally, I made one for custom components, there is possibly a better way to implement this into the same hook, but this is what I've scribbled fast:

export const useStyledCustom = (Component: React.ComponentType<any>) => {
    const ret = (css: string) => {
        const Inner = React.useRef(styled(Component)`
            ${(props: any) => props.css || ""}
        `).current;

        return React.useCallback(
            React.forwardRef((props: any, ref: any) => (
                <Inner ref={ref} {...props} css={css} />
            )),
            [css],
        );
    };
    return ret;
};

EDIT 2:
After more experimenting with it, I noticed it doesn't behave as a normal SC. Not sure how to explain it exactly, but take for example the transition property which doesn't have any effect because the component seems to be rebuilt from scratch instead. Not sure if this behavior was intended, maybe I'm just not using it how it's meant to be used for my case 😛

@jEnbuska
Copy link

jEnbuska commented Apr 5, 2019

I have been using styled-components for only few months now and I find it a bit cumbersome with react hooks. Is it still necessary to have functions as template string arguments now that we have hooks in react?

Wouldn't something like this match better with hooks pattern?:

function useButtonClassName({primary, size}) {
   const {buttonSize, colors, backgrounds} = useTheme();
   return useClassName([primary, size])`
       color: ${primary ? color.buttonPrimary, color.buttonSecondary};
       background: ${primary ? backgrounds.buttonPrimary : backgrounds.buttonSecondary};
       /* ...  */
   `
}

or

function useButtonClassName({primary, size}) {
   const {buttonSize, colors, backgrounds} = useTheme();
   return useClassName([primary, size], ({theme, className}) => className`
       color: ${primary ? color.buttonPrimary, color.buttonSecondary};
       background: ${primary ? backgrounds.buttonPrimary : backgrounds.buttonSecondary};
       /* ...  */
   `)
}

@export-mike
Copy link

It feels to me whenever a new pattern arises. "we must use this for everything!" doesn't make any sense to me. It would however make sense for just a couple of API's such as withTheme => equivalent hook => useTheme()

@quantizor
Copy link
Contributor

quantizor commented Apr 6, 2019 via email

@jEnbuska
Copy link

jEnbuska commented Apr 7, 2019

Something like:

const className = useStyle`
  color: ${p => p.theme.colors.red};
`;

Is that possible, using a TTL as a hook?

Would it make easier to reuse CSS if defining styles 'useStyles' would be separate from defining css class?

/* return a plain css string */
function useFormItemOutlineCSS(){
  const {focusOutlineColor} = useContext(ThemeProvider);
  return useCss([])`
    :focus {      
      box-shadow: 0 0 0 4px ${focusOutlineColor}
    }
  `;
}

/* return className string and inject style tag to document */
function useButtonClassName({primary}) {
   const formItemOutline: string = useFormItemOutlineCSS();
   const {button} = useContext(ThemeContext);
   return useClassName([primary])`
     ${formItemOutline};
     background: ${primary ? button.backgroundPrimary : button.backgroundSecondary};
     /* ... */  
  `;
}

@quantizor
Copy link
Contributor

@jEnbuska We have the css helper for that use case, if I'm understanding your example properly.

@sibelius
Copy link

is this enough to get theme?

import { useContext } from 'react';
import { ThemeContext } from 'styled-components';

export const useTheme = () => {
  const theme = useContext(ThemeContext);

  return theme;
};

@quantizor quantizor mentioned this issue Apr 19, 2019
14 tasks
@quantizor
Copy link
Contributor

Closed by #2390

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

10 participants