Update React CSS-in-JS styling guide

gomorizsolt commented 5 years ago

Issue #4.

Updated the example to our current styling format.

thisismydesign commented 5 years ago

The Store CSS-in-JS styles in separate files chapter also have to be updated. But first, do you remember what the issue was with using css from styled-components and why we went with styled.element instead?

gomorizsolt commented 5 years ago

If I'm right, the reason for the movement was the difficulty of writing tests in that format.

Btw, why'd you prefer the way that you originally described in the documentation over the one that we currently use?

thisismydesign commented 5 years ago

See the Store CSS-in-JS styles in separate files chapter.

Can you reproduce those difficulties?

gomorizsolt commented 5 years ago

Yes, I'll give that a try asap.

gomorizsolt commented 5 years ago

After investigating the styling in the recommended format, the main obstacle is that we can't test whether a particular styled component is rendered or not.

To stick with the example you created in the doc previously:

import styled from 'styled-components';
import { headerDivStyle } from './Header.style';

const header = () => {
  // This element isn't accessible in the tree. 
  const StyledHeader = styled.div`${headerDivStyle};`;

  return <StyledHeader />;
};

export default header;

We can either try to figure out a workaround to handle this issue or leave it as is - only if it's not a problem that the rendering of different components can't be tested.

thisismydesign commented 5 years ago

Thanks! Not sure if I understand the use case though, could you give me a test example that doesn't work in this case?

gomorizsolt commented 5 years ago

Yes, of course. Let's stick with the original example as before.

import styled from 'styled-components';
import { headerDivStyle } from './Header.style';

const header = () => {
  // This element isn't accessible in the tree. 
  const StyledHeader = styled.div`${headerDivStyle};`;

  return <StyledHeader />;
};

export default header;

If we'd like to test whether the <StyledHeader /> component is rendered or not, the test case should be something likewise the one below:

// Usual imports ...
import styled from 'styled-components';
import { headerDivStyle } from './Header.style';

describe('<Header />', () => {
   let headerWrapper;

   beforeEach(() => {
      headerWrapper = shallow(<Header />);
   });

   it('renders StyledHeader', () => {
      // Testing / accessing the StyledHeader component isn't possible here.
      // This isn't pointing to the element in the tree.
      const StyledHeader = styled.div`${headerDivStyle}`;

     // Failing ...
     expect(headerWrapper.find(StyledHeader)).toHaveLength(1);
   });
});

thisismydesign commented 5 years ago

Understood, thanks. And that makes sense I think.

So, in this case, we'd aim to test that a certain element is present with certain styles. Is that possible to do some other way? E.g. expect(headerWrapper.find(divElementMatcher)).toHaveStyle(headerDivStyle)

What made me think a lot is that I understand the appeal of trying to do what you mentioned above. And then the style could be tested separately form the component that uses it.

With that said, the exported style can be tested separately with the currently proposed css approach as well. And since we're only talking about simple http elements

they should be easy to find/match
there's no difference whether they are wrapped in a named export or defined on the fly in the component (i.e. whether you match something called StyledHeader which is a simple div or that you match an unnamed div)

gomorizsolt commented 5 years ago

That's absolutely right.

Hopefully there're some existing libraries or whatever that'd fit your example above(e.g. jest-styled-components).

To summarize the things:

we wouldn't like to change the way of defining CSS-IN-JS elements, hence the doc should be restored to the previous version,
figure out how to test styled components(take a look at the proposed package),
extend the doc with an additional example that demonstrates the convention(s) of testing styles separately.

thisismydesign commented 5 years ago

Sounds good. Finding the element should be easy: https://airbnb.io/enzyme/docs/api/ReactWrapper/find.html

I think there're 2 ways to test styles: 1, in Header.test.js find element, assert specific style with toHaveStyleRule. E.g.

expect(headerWrapper.find("div").toJSON()).toHaveStyleRule('color', 'red')

2, in Header.test.js find element, assert style class. then in Header.styles.test.js assert specific style rules of style class. E.g.

expect(headerWrapper.find("div").toJSON()).toHaveStyleRule(headerDivStyle)
expect(headerDivStyle).toHaveStyleRule('color', 'red')

I think there's no need for an additional redirection with option 2. It could be useful for testing global styles though. I'd go with option 1. Could you provide an example and make sure that it works?

gomorizsolt commented 5 years ago

I'd also go with option 1.

Yes, of course.

gomorizsolt commented 5 years ago

Unfortunately, the .toJSON() method can't be used likewise you did in the example.

TypeError: wrapper.find(...).toJSON is not a function

Otherwise, why is it necessary to find the element?

I assume that the answer is to test both the component rendering and also whether the desired style is applied to a particular component or not.

But this raises more concerns. How'd you find an element if there're more than one div in the tree? Enzyme provides the .first() and .at(index) methods to find the corresponding elements, but IMO that'd make the tests static(e.g. moving/removing a div requires overriding all the occurrences of .at(index)).

thisismydesign commented 5 years ago

I got that part from jest-styled-components but it is used differently indeed: renderer.create(<Button />).toJSON()

I would expect that a component wouldn't contain too many elements. If it did, they would be extracted to sub-components (and shallow rendering or simply looking on the first level would allow us to only look at the elements of the main component). Also, we can use IDs/classes to reference items.

But this is mostly speculation from my part, I'm not sure whether this is feasible in practice. Please look for an example in existing projects where this could be tested or keep this in mind for the upcoming projects and we can then revisit this issue.

gomorizsolt commented 5 years ago

I couldn't figure out how to find a particular styled element in the tree with Enzyme - IMO the way you proposed above isn't feasible. The solutions that I can come up with are either setting an id to the desired component or using the displayName property.

Examples

Solution 1 - set a specific ID to the element

// Header.style.js
export const HeaderButton = css`
  color: red;
`;

// Header.js
import React from "react";
import styled from "styled-components";
import { HeaderButton } from "./Header.style.js";

const header = () => {
   const StyledHeaderButton = styled.button`${HeaderButton}`;

   return <StyledHeaderButton id="headerBtn" />;
}

export default header;

// Header.test.js
// Importing the usual elements -> React, shallow, renderer, Header, HeaderButton etc.

describe("<Header />", () => {
   it("renders a red colored button", () => {
      const StyledHeaderButton = styled.button`${HeaderButton}`;

      const headerWrapper = shallow(<Header />);
      const styledHeaderButtonWrapper = renderer.create(<StyledHeaderButton />).toJSON();

      expect(headerWrapper.find("#headerBtn")).toHaveLength(1);
      expect(styledHeaderButtonWrapper).toHaveStyleRule("color", "red");
   });
});

Solution 2 - set the displayName property

The main difference from the previous example occurs in Header.js.

// ...
const header = () => {
   const StyledHeaderButton = styled.button`${HeaderButton}`;

   StyledHeaderButton.displayName = "StyledHeaderButton";

   return <StyledHeaderButton />;
}

export default header;

As a result, the element can be found likewise in the example below:

// ...
expect(headerWrapper.find("StyledHeaderButton")).toHaveLength(1);

In theory, the displayName can be set automatically, but I couldn't make it workable yet - although the setup is quite straightforward.

Personally, I'd prefer the second solution over the first one as it doesn't require adding the id attribute to each element - only because they're inaccessible in the tests.

thisismydesign commented 5 years ago

Testing that the rendered element has the correct styles would be the most important part, so we find a way to do that.

On Sun, Apr 28, 2019 at 12:27 PM Zsolt Gomori notifications@github.com wrote:

I couldn't figure out how to find a particular styled element in the tree with Enzyme - IMO the way you proposed above isn't feasible. The solutions that I can come up with are either setting an id to the desired component or using the displayName property. Examples

Solution 1 - set a specific ID to the element

// Header.style.js export const HeaderButton = css color: red; ;

// Header.js import React from "react"; import styled from "styled-components"; import { HeaderButton } from "./Header.style.js";

const header = () => {

const StyledHeaderButton = styled.button${HeaderButton};

return ;

}

export default header;

// Header.test.js // Importing the usual elements -> React, shallow, renderer, Header, HeaderButton etc.

describe("
", () => {

it("renders a red colored button", () => {
  const StyledHeaderButton = styled.button`${HeaderButton}`;

  const headerWrapper = shallow(<Header />);

  const styledHeaderButtonWrapper = renderer.create(<StyledHeaderButton />).toJSON();

  expect(headerWrapper.find("#headerBtn")).toHaveLength(1);

  expect(styledHeaderButtonWrapper).toHaveStyleRule("color", "red");
});

});

Solution 2 - set the displayName property

The main difference from the previous example occurs in Header.js.

// ... const header = () => {

const StyledHeaderButton = styled.button${HeaderButton};

StyledHeaderButton.displayName = "StyledHeaderButton";

return ;

}

export default header;

As a result, the element can be found likewise in the example below:

// ... expect(headerWrapper.find("StyledHeaderButton")).toHaveLength(1);

In theory, the displayName can be set automatically https://github.com/styled-components/styled-components/issues/896#issuecomment-356756789, but I couldn't make it workable yet - although the setup is quite straightforward.

Personally, I'd prefer the second solution over the first one as it doesn't require adding the id attribute to each element because they're inaccessible in the tests.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/c-hive/guides/pull/9#issuecomment-487366224, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZFU3A25R2MBFC2OUB3ACLPSV32LANCNFSM4HIBHPAQ .

gomorizsolt commented 5 years ago

Faced with an additional issue/proposal during applying these guides in React:

IMO it'd be better to agree on a common naming convention in the case of styled-components.

Example:

// => test.style.js
import { css } from "styled-components";

export const button = css`
   // styles ...
`;

// => test.js
import styled from "styled-components";
import { button } from "./test.style.js";

const StyledButton = styled.button`${button}`;

As you can see, the styled components are prefixed with the Styled word; however, I'm not sure if it's the right approach.

Furthermore, it's also confusing to see components with the same prefixes - i.e. it's hard to distinguish them.

How about using the following combination?

css prefix for css styles, e.g. cssButton,
UpperCamelCased components without any prefix, e.g. Button.

Is it something that should be considered?

For further practical examples, see the Add reusable UI elements PR.

thisismydesign commented 5 years ago

Good idea. Not sure where the Styled* convention came from, maybe some article, but it is indeed redundant. How about buttonStyle and Button?

gomorizsolt commented 5 years ago

Not sure where the Styled* convention came from, maybe some article, but it is indeed redundant.

I guess this was in one of my first implementations and sticked with that for some reason. :)

How about buttonStyle and Button?

Sounds good. Should this convention be included in the guides? If so, I'll create a separate PR.

thisismydesign commented 5 years ago

Yes please.

On Wed, Jun 19, 2019 at 8:46 PM Zsolt Gomori notifications@github.com wrote:

Not sure where the Styled* convention came from, maybe some article, but it is indeed redundant.

I guess this was in one of my first implementations and sticked with that for some reason. :)

How about buttonStyle and Button?

Sounds good. Should this convention be included in the guides? If so, I'll create a separate PR.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/c-hive/guides/pull/9?email_source=notifications&email_token=AAZFU3CTSWSU5JBJVKLLRVTP3J5HZA5CNFSM4HIBHPA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYCZP2Y#issuecomment-503683051, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZFU3BIYPHE6XSP3C5T7VDP3J5HZANCNFSM4HIBHPAQ .

thisismydesign commented 4 years ago

Closing in favor of https://github.com/c-hive/guides/commit/7aa176e2e090a6c6ed31a7cc949f2241a5f71a32

In the end, using styled makes sense. The reason I preferred css is that then there was no issue with passing props. However, props can be quite easily passed as shown in the new example.

c-hive / guides

Update React CSS-in-JS styling guide #9

Examples