Fix the padding bug 🎉 (#44)

* It works! * idk * fix the stories * fix padding warning * try new node modules workflow * fix a few more things
roginfarrer · Apr 12, 2020 · b67e76c · b67e76c
1 parent c14377e
commit b67e76c
Show file tree

Hide file tree

Showing 10 changed files with 233 additions and 220 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -2,30 +2,32 @@ name: CI
 on: [push]
 
 jobs:
-  job1:
-    name: Test
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@v1
-      - uses: borales/actions-yarn@v2.0.0
-        with:
-          # will run `yarn install` command
-          cmd: install
-      - uses: borales/actions-yarn@v2.0.0
-        with:
-          # will run `yarn test` command
-          cmd: test
-  job2:
-    name: Lint
-    needs: job1
+  build:
     runs-on: ubuntu-latest
+
     steps:
-      - uses: actions/checkout@v1
-      - uses: borales/actions-yarn@v2.0.0
-        with:
-          # will run `yarn install` command
-          cmd: install
-      - uses: borales/actions-yarn@v2.0.0
-        with:
-          # will run `yarn test` command
-          cmd: lint
+    - uses: actions/checkout@v2
+
+    - name: Cache node modules
+      uses: actions/cache@v1
+      env:
+        cache-name: cache-node-modules
+      with:
+        path: ~/.npm # npm cache files are stored in `~/.npm` on Linux/macOS
+        key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json') }}
+        restore-keys: |
+          ${{ runner.os }}-build-${{ env.cache-name }}-
+          ${{ runner.os }}-build-
+          ${{ runner.os }}-
+
+    - name: Install Dependencies
+      run: yarn install
+
+    - name: Build
+      run: yarn build
+
+    - name: Test
+      run: yarn test
+
+    - name: Lint
+      run: yarn lint
diff --git a/package.json b/package.json
@@ -47,7 +47,7 @@
     "@storybook/addons": "^5.3.12",
     "@storybook/react": "^5.3.12",
     "@testing-library/jest-dom": "^5.3.0",
-    "@testing-library/react": "^9.4.0",
+    "@testing-library/react": "^10.0.2",
     "@testing-library/react-hooks": "^3.2.1",
     "@types/jest": "^25.1.2",
     "@types/raf": "^3.4.0",

diff --git a/src/__tests__/hooks.test.tsx b/src/__tests__/hooks.test.tsx
@@ -1,61 +1,67 @@
 import React from 'react';
-import { renderHook, act } from '@testing-library/react-hooks';
-import { render } from '@testing-library/react';
+import { render, act } from '@testing-library/react';
 import { useEffectAfterMount, useControlledState } from '../hooks';
 
 describe('useEffectAfterMount', () => {
   it('does not run callback on first render', () => {
-    const cb = jest.fn();
-
     // Provide a dependency that changes, so it re-renders
     let x = 0;
-    const { rerender } = renderHook(() => {
+    const cb = jest.fn();
+
+    function UseEffectAfterMount() {
       x++;
-      return useEffectAfterMount(cb, [x]);
-    });
+      useEffectAfterMount(cb, [x]);
+      return null;
+    }
+
+    const { rerender } = render(<UseEffectAfterMount />);
 
     expect(cb).not.toHaveBeenCalled();
-    rerender();
+    rerender(<UseEffectAfterMount />);
     expect(cb).toHaveBeenCalled();
   });
 });
 
 describe('useControlledState', () => {
-  it('should match default snapshot', () => {
-    const {
-      result: { current },
-    } = renderHook(() => useControlledState({}));
-
-    expect(current).toMatchInlineSnapshot(`
-      Array [
-        false,
-        [Function],
-      ]
-    `);
+  let hookReturn: [boolean, () => void];
+
+  function UseControlledState({
+    defaultOpen,
+    isOpen,
+  }: {
+    defaultOpen?: boolean;
+    isOpen?: boolean;
+  }) {
+    const result = useControlledState({ defaultOpen, isOpen });
+
+    hookReturn = result;
+
+    return null;
+  }
+
+  it('returns a boolean and a function', () => {
+    render(<UseControlledState />);
+
+    expect(hookReturn[0]).toBe(false);
+    expect(typeof hookReturn[1]).toBe('function');
   });
 
   it('returns the defaultValue value', () => {
-    const { result } = renderHook(() =>
-      useControlledState({ defaultOpen: true })
-    );
-
-    const [value] = result.current;
+    render(<UseControlledState defaultOpen />);
 
-    expect(value).toBe(true);
+    expect(hookReturn[0]).toBe(true);
   });
 
   it('setter toggles the value', () => {
-    const { result } = renderHook(() =>
-      useControlledState({ defaultOpen: true })
-    );
+    render(<UseControlledState defaultOpen />);
 
-    expect(result.current[0]).toBe(true);
+    expect(hookReturn[0]).toBe(true);
 
     act(() => {
-      result.current[1]();
+      hookReturn[1]();
     });
 
-    expect(result.current[0]).toBe(false);
+    expect(hookReturn[0]).toBe(false);
   });
 
   describe('dev feedback', () => {
@@ -64,7 +70,6 @@ describe('useControlledState', () => {
     const originalWarn = console.warn;
     let consoleOutput: string[] = [];
     const mockWarn = (output: any) => consoleOutput.push(output);
-    console.warn = jest.fn(mockWarn);
 
     beforeEach(() => (console.warn = mockWarn));
     afterEach(() => {

diff --git a/src/__tests__/index.test.tsx b/src/__tests__/index.test.tsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import { render, fireEvent } from '@testing-library/react';
-import { renderHook, act } from '@testing-library/react-hooks';
+import { renderHook } from '@testing-library/react-hooks';
 import { mocked } from 'ts-jest/utils';
 import useCollapse from '../';
 import { getElementHeight } from '../utils';
@@ -42,27 +42,12 @@ test('does not throw', () => {
 
 test('returns expected constants', () => {
   const { result } = renderHook(useCollapse);
-  const {
-    isOpen,
-    getToggleProps,
-    getCollapseProps,
-    toggleOpen,
-  } = result.current;
-
-  expect(typeof isOpen).toBe('boolean');
-  expect(typeof toggleOpen).toBe('function');
-  expect(typeof getToggleProps()).toBe('object');
-  expect(typeof getCollapseProps()).toBe('object');
-});
 
-test('toggleOpen toggles isOpen', () => {
-  const { result } = renderHook(useCollapse);
-  const { toggleOpen } = result.current;
-  act(() => {
-    toggleOpen();
-  });
-  const { isOpen } = result.current;
-  expect(isOpen).toBe(true);
+  expect(result.current.isOpen).toStrictEqual(false);
+  expect(result.current.mountChildren).toStrictEqual(false);
+  expect(typeof result.current.toggleOpen).toBe('function');
+  expect(typeof result.current.getToggleProps()).toBe('object');
+  expect(typeof result.current.getCollapseProps()).toBe('object');
 });
 
 test('Toggle has expected props when closed (default)', () => {
@@ -178,23 +163,23 @@ describe('mountChildren', () => {
 });
 
 test('warns if using padding on collapse', () => {
-  // Even though the error is caught, it still gets printed to the console
-  // so we mock that out to avoid the wall of red text.
-  jest.spyOn(console, 'error');
-  // @ts-ignore
-  console.error.mockImplementation(() => {});
-
-  expect(() =>
-    render(
-      <Collapse
-        props={{ defaultOpen: true }}
-        collapseProps={{ style: { padding: 20 } }}
-      />
-    )
-  ).toThrowErrorMatchingInlineSnapshot(
-    `"Padding applied to the collapse element in react-collapsed will cause the animation to break, and never end. To fix, apply equivalent padding to the direct descendent of the collapse element."`
+  // Mocking console.warn so it does not log to the console,
+  // but we can still intercept the message
+  const originalWarn = console.warn;
+  let consoleOutput: string = '';
+  const mockWarn = (output: any) => (consoleOutput = output);
+  console.warn = jest.fn(mockWarn);
+
+  render(
+    <Collapse
+      props={{ defaultOpen: true }}
+      collapseProps={{ style: { padding: 20 } }}
+    />
+  );
+
+  expect(consoleOutput).toMatchInlineSnapshot(
+    `"Warning: react-collapsed: Padding applied to the collapse element will cause the animation to break and not perform as expected. To fix, apply equivalent padding to the direct descendent of the collapse element."`
   );
 
-  // @ts-ignore
-  console.error.mockRestore();
+  console.warn = originalWarn;
 });
diff --git a/src/hooks.ts b/src/hooks.ts
@@ -1,4 +1,4 @@
-import { useState, useRef, useEffect, useCallback } from 'react';
+import { RefObject, useState, useRef, useEffect, useCallback } from 'react';
 import warning from 'tiny-warning';
 
 export function useControlledState({
@@ -60,3 +60,29 @@ export function useUniqueId(): number {
   useEffect(() => setId(genId()), []);
   return id;
 }
+
+export function usePaddingWarning(element: RefObject<HTMLElement>): void {
+  // @ts-ignore
+  let warn = (el?: RefObject<HTMLElement>): void => {};
+
+  if (__DEV__) {
+    warn = el => {
+      if (!el?.current) {
+        return;
+      }
+      const { paddingTop, paddingBottom } = window.getComputedStyle(el.current);
+      const hasPadding =
+        (paddingTop && paddingTop !== '0px') ||
+        (paddingBottom && paddingBottom !== '0px');
+
+      warning(
+        !hasPadding,
+        'react-collapsed: Padding applied to the collapse element will cause the animation to break and not perform as expected. To fix, apply equivalent padding to the direct descendent of the collapse element.'
+      );
+    };
+  }
+
+  useEffect(() => {
+    warn(element);
+  }, [element]);
+}