Decisiv / styled-components-modifiers

A library to enable BEM flavored modifiers (and responsive modifiers) in styled components.
MIT License
298 stars 11 forks source link

Should use `css()` function for wrapping styles. #11

Closed alanbsmith closed 5 years ago

alanbsmith commented 6 years ago

This is an optimization suggested by the styled-components team.

andrewtpoe commented 6 years ago

For reference, this is the css function being discussed.

What are your thoughts on this as a solution?

  1. If a modifier is constructed so it either is a string or returns a string we call css(modifierStyleString) to ensure the css optimizations.
    // modifier is a string
    key: `
    // ...styles
    `
    // modifier returns a string
    key: () => `
    // ...styles
    `
  2. If a modifier returns a function, we call it & use the result directly:
    key: () => css`
    // ...styles
    `

    My concern with this approach is the following condition:

    key: css`
    // ...styles
    `

    This will need some experimentation to determine if the css function from styled-components returns a string or a different datatype. If it returns just a string (specifically when there are no interpolations), my proposed solution would essentially result in the css function being called twice.

The preference will be for developers to use css when writing the modifiers as this will enable other tools like stylelint and possibly prettier.

eugeneross commented 6 years ago

Any updates on this?

andrewtpoe commented 6 years ago

At present, no. If you'd like to use the css helper now you can return an object in the styled components modifiers:

  modifierName: () => ({ 
    styles: css`
      // your modifying styles
    `,
  })

This works fine in v1, although our plan is to deprecate the returned object pattern in v2.

eugeneross commented 6 years ago

@andrewtpoe Oh okay, thanks! I'll keep an eye on this.

On a side note, I have a couple questions for what you guys do at Decisiv. Would it be okay to ping you via email with the questions I have? 😄

mcmillion commented 5 years ago

I've been looking for a better way to test conditional modifiers on styled components without being coupled directly to the CSS in said modifiers (with something like .toHaveStyleRule()). I've gone down a few rabbit holes (trying to add modifiers to the components displayName, etc). One idea I thought of was to use reusable chunks of CSS (using css) and then writing some helpers to assert that those styles were being used when a given modifier was expected. The issue I ran into relates to this topic I think.

Currently, it appears that the way props are applied to the CSS used by applyStyleModifiers differs from how styled components does it. For example, the following won't work:

const ButtonPrimary = css`
  background: ${props => props.theme.primaryColor};
`;

const BUTTON_MODIFIERS = {
  primary: () => `${ButtonPrimary}`,
}

I've tried various other ways, but I haven't found any that work. It seems that the way props are handled by applyStyleModifiers is different than how they're actually handled inside styled components (which makes sense given all of the examples use ${props.whatever} rather than {props => props.whatever}.

Seems like this would be a breaking change to correct the behavior, however.

andrewtpoe commented 5 years ago

We have two utilities that we've built internally to help with both building and testing styled components + modifiers.

Our exact buildStyledComponent utility is not open sourced, but you can see how the buildStyledComponent util here works. It's very similar.

One of the steps we do is add the modifiers as a property to the styled component. In our test util, we then iterate over the component with a modifier applied. On each iteration, we take a snapshot of the component. We don't use snapshots for much other than this, but for verifying styles are consistent we've found it to be a good fit.

Our testStyledComponent util looks something like this:

import 'jest-styled-components';

import React from 'react';
import renderer from 'react-test-renderer';

export default function testStyledComponent(
  Component,
  testProps = {},
  testCaseTitle = Component.displayName,
) {
  describe(testCaseTitle, () => {
    it('applies the correct base styles', () => {
      const tree = renderer.create(<Component {...testProps} />).toJSON();
      expect(tree).toMatchSnapshot();
    });

    if (Component.modifiers) {
      const modifiers = Object.keys(Component.modifiers);
      modifiers.forEach(modifier => {
        it(`applies ${modifier} styles correctly`, () => {
          const tree = renderer
            .create(<Component {...testProps} modifiers={[modifier]} />)
            .toJSON();
          expect(tree).toMatchSnapshot();
        });
      });
    }
  });
}

This combo makes testing our styled components + modifiers a single line of code.

andrewtpoe commented 5 years ago

This issue was resolved in #20.