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

WIP: Formatic 3000 #124

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
230f394
Draft of futureland spec
jdeal Apr 6, 2019
8eec2ce
Add future demo page
jdeal Apr 6, 2019
cebd6fa
Add simple form example/test/code.
jdeal Apr 6, 2019
8f390f4
FormContainer works either as controlled/uncontrolled
jdeal Apr 6, 2019
2c827b3
Rename some files
jdeal Apr 6, 2019
c5d0bc8
Fix test descriptions
jdeal Apr 6, 2019
6739955
Add FieldContainer example/test/code
jdeal Apr 6, 2019
f23a600
Add a built-in TextInput
jdeal Apr 6, 2019
3fe0ad1
Add built-in TextField per the spec
jdeal Apr 6, 2019
f193215
Cleanup tests
jdeal Apr 6, 2019
af6e3e0
Auto-generate unique input ids
jdeal Apr 6, 2019
0fa226e
Clean up test descriptions
jdeal Apr 6, 2019
067a500
Add support for renderTag
jdeal Apr 9, 2019
8878def
Fix linting errors
jdeal Apr 9, 2019
0f4c5d5
Remove reliance on symbols for renderKeys
jdeal Apr 9, 2019
0c571f3
Add accessibilty to problems/goals
jdeal Apr 9, 2019
018b2b6
Add support for renderComponent
jdeal Apr 10, 2019
221b69c
Split up monolithic index
jdeal Apr 10, 2019
2476fc3
Support nested values
jdeal Apr 10, 2019
ace4df5
Simplify UncontrolledContainer
jdeal Apr 11, 2019
565da6f
Remove private components from API
jdeal Apr 11, 2019
be8de3c
Make sure uncontrolled demo forms do not rerender
jdeal Apr 20, 2019
7e5f9bd
Check if `onChange` is a function before calling it
jdeal Apr 20, 2019
abe9f0c
Add failing test for avoiding rerendering
jdeal Apr 20, 2019
737daa6
Add ReactiveValueContainer/useReactiveValue primitives
jdeal Apr 24, 2019
dfc5f54
Use ReactiveValue to implement more performant useField
jdeal Apr 24, 2019
9cc81ac
Add IntegerField/IntegerInput
jdeal Apr 25, 2019
9218c37
Remove unused fields
jdeal Apr 25, 2019
c84bc32
Add AutoFields. Move field-components to their own folder.
Apr 27, 2019
6aeaf24
Remove AutoFields default in FormContainer
Apr 29, 2019
7d7a3ad
Better tests
Apr 29, 2019
9b6362d
Use RenderContext instead of ReactiveValue to get initial form value
Apr 29, 2019
0185cdd
Rename files/dirs
Apr 29, 2019
089c4f3
Merge pull request #126 from zapier/add-autofields
jdeal May 4, 2019
06c06b4
Allow using the whole value
jdeal May 4, 2019
4bc8326
Just a naming tweak
jdeal May 4, 2019
65f7c08
Even more performance ReactiveValue with metadata
jdeal May 4, 2019
87660c8
Use useReactiveValueMeta for AutoFields
jdeal May 4, 2019
a4a0242
Update meta when root value changes
May 9, 2019
4d3981b
Bump zapier-scripts to current
May 9, 2019
302cf96
Add jest-dom and convert ReactiveValue tests
May 9, 2019
0b80b23
Add eslint override to allow react-create-class
May 9, 2019
a08c04a
Ensure we have meta when updating
May 10, 2019
5e9b175
Merge pull request #127 from zapier/notify-meta-on-value
stevelikesmusic May 10, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions __tests__/future/AvoidRerendering.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import { FormContainer, TextField, useField } from '@/src/future';

afterEach(cleanup);

describe('avoid rerendering', () => {
const defaultValue = { firstName: '', lastName: '' };

test('should avoid unnecessary rerendering with uncontrolled container', async () => {
const renderSpy = jest.fn();
function FirstName() {
renderSpy('firstName');
const { value } = useField('firstName');
return <div>First Name: {value}</div>;
}
function LastName() {
renderSpy('lastName');
const { value } = useField('lastName');
return <div>First Name: {value}</div>;
}
const { getByLabelText } = render(
<FormContainer defaultValue={defaultValue}>
<FirstName />
<LastName />
<TextField fieldKey="firstName" id="firstName" label="First Name" />
<TextField fieldKey="lastName" id="lastName" label="Last Name" />
</FormContainer>
);
fireEvent.change(getByLabelText('First Name'), {
target: { value: 'Joe' },
});
expect(renderSpy.mock.calls).toEqual([
['firstName'],
['lastName'],
['firstName'],
]);
});
});
30 changes: 30 additions & 0 deletions __tests__/future/BuiltInField.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import { ExampleForm, defaultValue } from '@/demo/future/BuiltInField';

afterEach(cleanup);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the setupTestFrameworkScriptFile option in jest.config would call setup cleanup for all tests. Then we wouldn't need to explicitly call it in every test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that seems reasonable. The only downside of that is you can't opt out of that behavior then? For example, if you wanted to do some iterative tests on the same DOM, you could use afterAll, but with the global setting, your DOM would be wiped out each time.


describe('built-in field', () => {
test('should fire onChange correctly', () => {
const onChangeSpy = jest.fn();
const { getByLabelText } = render(
<ExampleForm defaultValue={defaultValue} onChange={onChangeSpy} />
);
fireEvent.change(getByLabelText('First Name'), {
target: { value: 'Joe' },
});
expect(onChangeSpy.mock.calls[0][0]).toEqual({
firstName: 'Joe',
lastName: '',
});
fireEvent.change(getByLabelText('Last Name'), {
target: { value: 'Foo' },
});
expect(onChangeSpy.mock.calls[1][0]).toEqual({
firstName: 'Joe',
lastName: 'Foo',
});
});
});
62 changes: 62 additions & 0 deletions __tests__/future/BuiltInFieldAutoInputId.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import { FormContainer, TextField } from '@/src/future';

afterEach(cleanup);

describe('built-in field with auto input ids', () => {
test('should have auto-generated input ids', () => {
const onChangeSpy = jest.fn();
const defaultValue = {
firstName: '',
lastName: '',
};

const renderForm = () => (
<FormContainer defaultValue={defaultValue} onChange={onChangeSpy}>
<div data-testid="firstNameWrapper">
<TextField fieldKey="firstName" label="First Name" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach a lot - you can specify your fields with JSX and control where it goes, add wrapper stuff, etc.

I also really like the useField hook. I think that's a great way to write custom components or extend things.

Just my first impressions, but this is all feeling very nice!

</div>
<div data-testid="lastNameWrapper">
<TextField fieldKey="lastName" label="Last Name" />
</div>
</FormContainer>
);
const { getByTestId, rerender } = render(renderForm());

const firstNameInput = getByTestId('firstNameWrapper').querySelector(
'input'
);
const lastNameInput = getByTestId('lastNameWrapper').querySelector('input');

const firstNameId = firstNameInput.getAttribute('id');
const lastNameId = lastNameInput.getAttribute('id');

expect(firstNameId).toBeTruthy();
expect(lastNameId).toBeTruthy();
expect(firstNameId).not.toEqual(lastNameId);

fireEvent.change(firstNameInput, {
target: { value: 'Joe' },
});
expect(onChangeSpy.mock.calls[0][0]).toEqual({
firstName: 'Joe',
lastName: '',
});
fireEvent.change(lastNameInput, {
target: { value: 'Foo' },
});
expect(onChangeSpy.mock.calls[1][0]).toEqual({
firstName: 'Joe',
lastName: 'Foo',
});

rerender(renderForm());

// Ids should not change when rerendering.
expect(firstNameId).toEqual(firstNameInput.getAttribute('id'));
expect(lastNameId).toEqual(lastNameInput.getAttribute('id'));
});
});
30 changes: 30 additions & 0 deletions __tests__/future/BuiltInInput.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import { ExampleForm, defaultValue } from '@/demo/future/BuiltInInput';

afterEach(cleanup);

describe('built-in input', () => {
test('should fire onChange correctly', () => {
const onChangeSpy = jest.fn();
const { getByLabelText } = render(
<ExampleForm defaultValue={defaultValue} onChange={onChangeSpy} />
);
fireEvent.change(getByLabelText('First Name'), {
target: { value: 'Joe' },
});
expect(onChangeSpy.mock.calls[0][0]).toEqual({
firstName: 'Joe',
lastName: '',
});
fireEvent.change(getByLabelText('Last Name'), {
target: { value: 'Foo' },
});
expect(onChangeSpy.mock.calls[1][0]).toEqual({
firstName: 'Joe',
lastName: 'Foo',
});
});
});
35 changes: 35 additions & 0 deletions __tests__/future/ControlledCustomFormWithHooks.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import {
ExampleForm,
value,
} from '@/demo/future/ControlledCustomFormWithHooks';

afterEach(cleanup);

describe('controlled custom form with hooks', () => {
test('should fire onChange correctly', () => {
const onChangeSpy = jest.fn();
const { getByLabelText, rerender } = render(
<ExampleForm onChange={onChangeSpy} value={value} />
);
fireEvent.change(getByLabelText('First Name'), {
target: { value: 'Joe' },
});
const newValue = onChangeSpy.mock.calls[0][0];
expect(newValue).toEqual({
firstName: 'Joe',
lastName: '',
});
rerender(<ExampleForm onChange={onChangeSpy} value={newValue} />);
fireEvent.change(getByLabelText('Last Name'), {
target: { value: 'Foo' },
});
expect(onChangeSpy.mock.calls[1][0]).toEqual({
firstName: 'Joe',
lastName: 'Foo',
});
});
});
33 changes: 33 additions & 0 deletions __tests__/future/CustomFormWithFieldContainer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import {
ExampleForm,
defaultValue,
} from '@/demo/future/CustomFormWithFieldContainer';

afterEach(cleanup);

describe('custom form with FieldContainer', () => {
test('should fire onChange correctly', () => {
const onChangeSpy = jest.fn();
const { getByLabelText } = render(
<ExampleForm defaultValue={defaultValue} onChange={onChangeSpy} />
);
fireEvent.change(getByLabelText('First Name'), {
target: { value: 'Joe' },
});
expect(onChangeSpy.mock.calls[0][0]).toEqual({
firstName: 'Joe',
lastName: '',
});
fireEvent.change(getByLabelText('Last Name'), {
target: { value: 'Foo' },
});
expect(onChangeSpy.mock.calls[1][0]).toEqual({
firstName: 'Joe',
lastName: 'Foo',
});
});
});
30 changes: 30 additions & 0 deletions __tests__/future/CustomFormWithHooks.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*global jest, describe, test, expect, afterEach */
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import { ExampleForm, defaultValue } from '@/demo/future/CustomFormWithHooks';

afterEach(cleanup);

describe('custom form with hooks', () => {
test('should fire onChange correctly', () => {
const onChangeSpy = jest.fn();
const { getByLabelText } = render(
<ExampleForm defaultValue={defaultValue} onChange={onChangeSpy} />
);
fireEvent.change(getByLabelText('First Name'), {
target: { value: 'Joe' },
});
expect(onChangeSpy.mock.calls[0][0]).toEqual({
firstName: 'Joe',
lastName: '',
});
fireEvent.change(getByLabelText('Last Name'), {
target: { value: 'Foo' },
});
expect(onChangeSpy.mock.calls[1][0]).toEqual({
firstName: 'Joe',
lastName: 'Foo',
});
});
});
43 changes: 43 additions & 0 deletions __tests__/future/NestedValues.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';

import { ExampleForm, defaultValue } from '@/demo/future/NestedValues';

afterEach(cleanup);

describe('nested values with FieldContainer', () => {
test('should fire onChange correctly', () => {
const onChangeSpy = jest.fn();
const { getByLabelText } = render(
<ExampleForm defaultValue={defaultValue} onChange={onChangeSpy} />
);
fireEvent.change(getByLabelText('First Name'), {
target: { value: 'Joe' },
});
expect(onChangeSpy.mock.calls[0][0]).toEqual({
name: { firstName: 'Joe', lastName: '' },
username: '',
});
fireEvent.change(getByLabelText('Last Name'), {
target: { value: 'Foo' },
});
expect(onChangeSpy.mock.calls[1][0]).toEqual({
name: {
firstName: 'Joe',
lastName: 'Foo',
},
username: '',
});
fireEvent.change(getByLabelText('Username'), {
target: { value: 'joefoo' },
});
expect(onChangeSpy.mock.calls[2][0]).toEqual({
name: {
firstName: 'Joe',
lastName: 'Foo',
},
username: 'joefoo',
});
});
});
28 changes: 28 additions & 0 deletions __tests__/future/StyledBuiltInField.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*global jest, describe, test, expect, afterEach*/
import React from 'react';
import { render, cleanup } from 'react-testing-library';

import { ExampleForm, defaultValue } from '@/demo/future/StyledBuiltInField';

afterEach(cleanup);

describe('styled built-in field', () => {
test('should use emotion to override styling', () => {
const onChangeSpy = jest.fn();
const { getByLabelText } = render(
<ExampleForm defaultValue={defaultValue} onChange={onChangeSpy} />
);
// Example upper-cases labels, and
// Emotion generates a class with "css-" in it, so look for that.
expect(
getByLabelText('FIRST NAME')
.getAttribute('class')
.indexOf('css-')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor—another way to get this might be:

expect(
  getByLabelText('FIRST NAME').className.includes('css-')
).toBe(true);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that's a little nicer!

).toBeGreaterThan(-1);
expect(
getByLabelText('LAST NAME')
.getAttribute('class')
.indexOf('css-')
).toBeGreaterThan(-1);
});
});
49 changes: 49 additions & 0 deletions demo/future/BoilerplateReactForm.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React, { useState } from 'react';

function TextField({ fieldKey, label, value, onChangeField }) {
return (
<div>
<div>
<label htmlFor={fieldKey}>{label}</label>
</div>
<div>
<input
id={fieldKey}
onChange={event => onChangeField(fieldKey, event.target.value)}
type="text"
value={value}
/>
</div>
</div>
);
}

export function ExampleForm({ defaultValue, onChange }) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question regarding syntax: does Formatic keep the same style guidelines we try to use company-wide? We say to always use arrow functions unless we need to keep context of this through the prototype chain, so as to document this's usage in a function.

I'm all for function ExampleForm, but just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does use Zapier's linting, but I don't think it's stuck with all of our internal style rules.

This was just something I was trying though. I've found myself liking function more than arrow functions in a lot of cases. Using function makes it easier to export as a default for example.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree

const [formState, setFormState] = useState(defaultValue);
const onChangeField = (key, value) => {
const newFormState = {
...formState,
[key]: value,
};
setFormState(newFormState);
onChange(newFormState);
};
return (
<form>
<TextField
fieldKey="firstName"
label="First Name"
onChangeField={onChangeField}
value={formState.firstName}
/>
<TextField
fieldKey="lastName"
label="Last Name"
onChangeField={onChangeField}
value={formState.lastName}
/>
</form>
);
}

export const defaultValue = { firstName: '', lastName: '' };
Loading