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

Development for 2.0 #17

Merged
merged 28 commits into from
May 18, 2018
Merged

Development for 2.0 #17

merged 28 commits into from
May 18, 2018

Conversation

alvinsj
Copy link
Contributor

@alvinsj alvinsj commented Sep 22, 2017

2.0.0:

@@ -42,7 +42,7 @@ export const inputValueLookup = (serializedValues, inputName) => {
var attrName = Object.keys(inputNameTree)[0]; // inputName describes only single path

var next = inputNameTree[attrName];
if (next == null) return null; // if it's already leaf, then no chance to be found.
if (next == null) return undefined; // if it's already leaf, then no chance to be found.
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

the returned value should be fixed to undefined when it's not found. (related issue #18)

className: 'field',
field: def,
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

field is redundant, as fieldProps is already merged in line 83. related issue #19

disabled: disabled
disabled: disabled,

defaultValue: typeof value !== 'undefined' ? undefined : defaultValue,
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

if there is value, defaultValue should not be used, this will cause a react warning/error complaining about both should not exist at the same time

related issue: #21

label: label,
error: error,
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

error implementation is too narrow here. To get error, we can make use of the new key formProps

related issue: #19 #20

disabled: disabled,

defaultValue: typeof value !== 'undefined' ? undefined : defaultValue,
formProps: formProps
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

this is a new key: formProps allows passing of parent props.
The useful use case could be passing errors, or calling change handler e.g. onFieldValueChanged(name, e)

related issue: #20

: inputValueLookup(formProps.defaultValues || {}, name);

// fieldProps.value > formProps.values(field).value
const value = typeof fieldProps.value !== 'undefined'
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

In the refactoring, it reduces the inherited properties to only 3 important ones:

  1. disabled - form.disabled should disable all child fields
  2. defaultValue - extracted form.props.defaultValues
  3. value - extracted from form.props.values

related issue #19

export const layout = (...children) => {
return (
<div key={`layout-${layoutCount++}`} className="layout">
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

this causes re-rendering bug, form is re-rendered everytime to a new number when props changes.

related issue: #8, #22

@@ -19,7 +17,7 @@ export const section = (name, ...rows) => {
);
});
return (
<div key={`section-${sectionCount++}`} className="section">
Copy link
Contributor Author

@alvinsj alvinsj Sep 22, 2017

Choose a reason for hiding this comment

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

this one too causes re-rendering.

related issue: #8, #22

@alvinsj alvinsj merged commit bdbd574 into master May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant