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.

Sign up for GitHub

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

Jump to bottom

Nested fields support #33

Merged
merged 23 commits into from
Mar 5, 2020
Merged

Nested fields support #33

merged 23 commits into from
Mar 5, 2020

Conversation

stellarhoof
Copy link
Member

@stellarhoof stellarhoof commented Feb 10, 2020 •
edited
Loading

Dragons be here

@stellarhoof stellarhoof self-assigned this Feb 10, 2020
@stellarhoof stellarhoof changed the title Nested array fields support [WIP] Nested array fields support Feb 12, 2020
@stellarhoof stellarhoof changed the title [WIP] Nested array fields support [WIP] Nested fields support Feb 12, 2020
@stellarhoof stellarhoof changed the title [WIP] Nested fields support Nested fields support Feb 14, 2020
@smartprocure smartprocure deleted a comment from decrapifier Feb 17, 2020
@smartprocure smartprocure deleted a comment from decrapifier Feb 17, 2020
@decrapifier
Copy link
Contributor

decrapifier commented Feb 17, 2020 •
edited
Loading

Warnings
⚠️ ❗ This PR is BIG (+535 -89) Please keep it below 500 net changes
⚠️ The README has not been updated. Please update the README.
Messages
📖 Could not find any browser results.

Generated by 🚫 dangerJS against b6d3012

daedalus28
daedalus28 suggested changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@daedalus28 daedalus28 left a comment

Choose a reason for hiding this comment

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

general thing is to simplify with more futil tree usage (even if that means improving futil tree methods)

src/util.js Outdated
import F from 'futil'
import _ from 'lodash/fp'

export let tokenizePath = path =>
Copy link
Contributor

Choose a reason for hiding this comment

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

You already have a futil tree configured, so you can just use the futil methods (e.g. Tree.lookup, etc)

Copy link
Member Author

@stellarhoof stellarhoof Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It turns out I still need this fn, because remove takes a string like remove('path.to.["dotted.field"]') and passing an array all the time is cumbersome

src/index.js Outdated
reset() {
node.value = F.when(_.isUndefined, '')(x.value)
if (_.isEmpty(path)) form.submit.state.error = null // Lil hack
Copy link
Contributor

Choose a reason for hiding this comment

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

just a reminder about our call, reset only needs to set values and not fields

src/index.js Outdated
x.value = node.value
node.errors = undefined
F.setOn(['value', ...path], toJS(node.value), baseNode)
F.setOn(['fields', ...path], toJS(node.fields), baseNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

this also goes away

src/index.js Outdated
clean() {
x.value = node.value
node.errors = undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

this goes away also

src/index.js Outdated
..._.omit('value', x),
remove(x) {
let parentPath = tokenizePath(x)
let path = parentPath.splice(parentPath.length - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

splitAt(-1) or something 😄

Copy link
Member Author

@stellarhoof stellarhoof Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I just discovered chunk from lodash so this can be done like

let tokens = tokenizePath(x)
let [parentPath, path] = tokens.length === 1 ? [[], tokens] : _.chunk(tokens.length - 1, tokens)

@stellarhoof stellarhoof force-pushed the feature/array-fields branch 2 times, most recently from 69ffe75 to 0df6dbb Compare February 25, 2020 18:15
@stellarhoof stellarhoof force-pushed the feature/array-fields branch 3 times, most recently from 2c9002e to e442d15 Compare February 25, 2020 21:10
daedalus28
daedalus28 suggested changes Mar 2, 2020
View reviewed changes
src/index.js Outdated
let saved = {}

let state = observable({
// config.field values are more of default values than anything else
Copy link
Contributor

Choose a reason for hiding this comment

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

this is confusing because you'd expect more specific values to take precedent over a top level value

Copy link
Contributor

Choose a reason for hiding this comment

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

after talking again about defaultValue on array items, there is good reason for this behavior but we should document it )(because parent values should take precedent for arrays because the children values would be defaults and not have a value for each child)

src/index.js Outdated
},
reset(value) {
// Reset errors
if (_.isEmpty(node.path)) clearFormErrors()
Copy link
Contributor

Choose a reason for hiding this comment

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

these seem like they should be the same code path - clearing errors on a node should traverse its children (e.g. clearing b in {a:{b:{c}}} should clear c also

src/index.js Outdated
if (_.isEmpty(node.path)) clearFormErrors()
else F.unsetOn([dotPath], state.errors)
// Reset value
if (!_.isUndefined(value)) F.setOn(valuePath, clone(value), saved)
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think we have a real use case for this - let's remove until we do

src/index.js Outdated
F.walk(x => x.fields)(x => {
let valuePath = ['value', ...x.path]
if (x.itemField) x.fields = initFields(makeItemFields(x), x.path)
if (_.isUndefined(x.value)) x.value = clone(x.defaultValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not have both value and defaultValue, since resetting resets to the stored value

src/index.js Outdated
if (x.itemField) x.fields = initFields(makeItemFields(x), x.path)
if (_.isUndefined(x.value)) x.value = clone(x.defaultValue)
if (_.isUndefined(_.get(valuePath, saved)))
F.setOn(valuePath, clone(x.defaultValue), saved)
Copy link
Contributor

Choose a reason for hiding this comment

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

see above about defaultValue

src/index.js Outdated
else if (_.isPlainObject(toJS(x.value))) node.value = {}
else node.value = ''
add(x) {
throwIfLeaf(node)
Copy link
Contributor

Choose a reason for hiding this comment

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

lets just not add add to leaf nodes - same with remove

src/index.js Outdated
let field = node.getField(parentPath)
field && field.remove(path)
} else {
throwIfLeaf(node)
Copy link
Contributor

Choose a reason for hiding this comment

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

lets just not add remove to leaves

dubiousdavid
dubiousdavid approved these changes Mar 5, 2020
View reviewed changes
Copy link
Member

@dubiousdavid dubiousdavid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks really good to me. Nice work!

@daedalus28 daedalus28 merged commit 3d9be6c into master Mar 5, 2020
@daedalus28 daedalus28 deleted the feature/array-fields branch March 5, 2020 06:18
@stellarhoof stellarhoof mentioned this pull request Mar 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daedalus28 daedalus28 daedalus28 approved these changes

@dubiousdavid dubiousdavid dubiousdavid approved these changes

@michsa michsa Awaiting requested review from michsa

Assignees

@stellarhoof stellarhoof

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stellarhoof @decrapifier @daedalus28 @dubiousdavid