cloudflare / cf-ui

:gem: Cloudflare UI Framework
Other
1.29k stars 81 forks source link

Snapshoting styles (CSS classes) #271

Closed tajo closed 7 years ago

tajo commented 7 years ago

Solves https://github.com/cloudflare/cf-ui/issues/242.

I've added snapshots of styles so we can see what property exactly changed, not only that something (classname) changed. That makes snapshots really useful. Here's the example what happened when I replaced

const styles = ({ theme }) => ({
  background: theme.background,
  border: theme.border,
  boxShadow: theme.boxShadow
});

by

const styles = ({ theme }) => ({
  background: theme.background,
  border: 'yellow',
  boxShadow: theme.boxShadow
});

in <Form>:

● should render vertical layout

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot 1.

-     Snapshot
+     Received

     <form
-      className="od1cvd"
+      className="nfgsvf"
       onSubmit={[Function]}
     >
       Form
     </form>

      at Object.<anonymous> (packages/cf-component-form/test/Form.js:19:30)
      at Promise.resolve.then.el (node_modules/p-map/index.js:42:16)
      at process._tickCallback (internal/process/next_tick.js:109:7)

  ● should render vertical layout

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot 2.

-     Snapshot
+     Received

     "
-    .od1cvd {
+    .nfgsvf {
       background: white;
-      border: 1px solid #dedede;
+      border: yellow;
       box-shadow: 0 1px 1px rgba(0, 0, 0, 0.05)
     }"

      at Object.<anonymous> (packages/cf-component-form/test/Form.js:20:27)
      at Promise.resolve.then.el (node_modules/p-map/index.js:42:16)
      at process._tickCallback (internal/process/next_tick.js:109:7)

The first part is the current state. But the second part actually tells you what property has changed in that CSS class. Isn't that cool, guys? ✋ Here's the way how you can write such a test:

import React from 'react';
import { Form } from '../src/index';
import { felaSnapshot } from 'cf-style-provider';

test('should render horizontal layout', () => {
  const snapshot = felaSnapshot(
    <Form layout="horizontal" onSubmit={() => console.log('submit')}>
      Form
    </Form>
  );
  expect(snapshot.component).toMatchSnapshot();
  expect(snapshot.styles).toMatchSnapshot();
});

Why to save two separate snapshots and not just

expect(snapshot).toMatchSnapshot();

? This way it preserves nice indentation. And those two snapshots are always displayed right after each other.

Also, I moved felaTestContext() and this new felaSnapshot() into cf-style-provider so we can easily share the same thing in next na cf-ux.

The last thing, I've removed cf- prefix, so all classnames can be shorter. Saving bytes.

koddsson commented 7 years ago

Any way we can ignore the class name alltogether? Can't think of a situation where we'd care about that.

sejoker commented 7 years ago

Great work @tajo, final piece in place. I agree with @koddsson, we care only about expect(snapshot.styles).toMatchSnapshot(); to tedious to type 2 lines instead of 1.

tajo commented 7 years ago

Any way we can ignore the class name alltogether? Can't think of a situation where we'd care about that.

There is no way to ignore them. We really need them too since you can have multiple fela generated classNames in one component, so you need to know what is what. Like here.

I agree with @koddsson, we care only about expect(snapshot.styles).toMatchSnapshot();

Wait, why don't we care about snapshot of React component itself? Imo it's still the more important one since broken styles are not nice but broken component is disastrous.

to tedious to type 2 lines instead of 1.

Small price for a perfect coverage and it's still simpler than it was. You don't have to import react-test-renderer and wrap everything with renderer.create().

koddsson commented 7 years ago

Any way we can ignore the class name alltogether? Can't think of a situation where we'd care about that.

There is no way to ignore them. We really need them too since you can have multiple fela generated classNames in one component, so you need to know what is what. Like here.

I figured that'd be the case, no worries :)