Conversation
DJWassink
force-pushed
the
feature/pristine
branch
from
4070d2e to
75ab99b
Compare
src/business/form.ts
Outdated
| this.triggerFieldListeners(fieldName); | ||
| } | ||
|
|
||
| private updateFieldValue(fieldName: string, value: any, pristineValue) { |
There was a problem hiding this comment.
implicit any on pristine value
src/business/form.ts
Outdated
| } | ||
|
|
||
| public setFieldValues(values: { [fieldName: string]: any }) { | ||
| public setFieldValues(values: { [fieldName: string]: any }, pristineValues = false) { |
There was a problem hiding this comment.
I'm not digging the magic third boolean, why not introduce something like a resetFieldValues method
DJWassink
force-pushed
the
feature/pristine
branch
from
51cc2ae to
af009d8
Compare
| this.triggerMultipleFieldListeners(fieldNames); | ||
| } | ||
|
|
||
| public initialiseFieldValue(fieldName: string, value: any) { |
There was a problem hiding this comment.
uk vs us? which do we choose? initialize is US definition with a s is UK
There was a problem hiding this comment.
Ah true, I think we should opt for US, I'm not so much of a tea drinker
| this.triggerFieldListeners(fieldName); | ||
| } | ||
|
|
||
| private updateFieldValue(fieldName: string, value: any, pristineValue: boolean) { |
There was a problem hiding this comment.
I'm expecting the value to be named pristineValue containing the value, do you mean isPristineValue?
|
|
||
| public state: { value: any } = { | ||
| value: this.props.form.getFieldValue(this.getFieldName()) | ||
| value: (typeof this.props.form.getFieldState(this.getFieldName()) !== 'undefined') |
There was a problem hiding this comment.
Might be better to put this in a seperate method, unreadable like this, if you do that you can extract the getFieldState result to a seperate variable and remove the need for a second call to the method
| } | ||
|
|
||
| public render() { | ||
| const fieldState = this.props.form.getFieldState(this.getFieldName()); |
There was a problem hiding this comment.
Why are we storing a copy of the value if we're still going to call getFieldState in the render method?
There was a problem hiding this comment.
Good question. This is following the initial design we had and maybe should change. The getFieldState gets called in favor of the getValidationError we had before. Do we wan't to store the validationError localy also?
There was a problem hiding this comment.
This might be the textbook use-case for forceUpdate, since our state is in another place.. so we can just remove local state and call the getFieldState in the render function https://facebook.github.io/react/docs/react-component.html#forceupdate
There was a problem hiding this comment.
So we would push the local state to the bonn form, so instead of setting the local state in the keyPressHandler we push it to bonn and let it forceUpdate any listening components? Sounds a bit tricky tho. Gotta be carefull to not render too much (e.g. having mutliple things inside a field would already force multiple renders). But I guess that would also mean a user mis-used the field decorator.
There was a problem hiding this comment.
Ehm, this is already what we're doing.. I just say we remove the state from the Field and Listener components and introduce a this.forceUpdate() within the listenForFieldChange callback. Then in the render we just fetch the values we need.
2 participants
🎉 🌮