-
Notifications
You must be signed in to change notification settings - Fork 18
remove ref from for get the data in TargetContainer #2625
Conversation
@@ -41,6 +41,7 @@ export default class Target extends Component { | |||
label: countTarget !== 0 ? i18n.t('label.update') : i18n.t('label.save'), | |||
initialValues: { countTarget, targetYear, targetComment } | |||
}; | |||
this.formRef=React.createRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I am looking for a universal solution we can use throughout our whole codebase I would like to ask what is the difference of this solution vs.
treecounter-app/app/components/Authentication/SignUp/SignUp.js
Lines 145 to 165 in 4ab1b4d
<form onSubmit={this.props.onSignUpClicked.bind(this, type, this.signupForm)}> | |
<TCombForm | |
ref={ref => (this.signupForm = ref)} | |
type={signupFormSchema[type]} | |
options={this.props.schemaOptions[type]} | |
value={this.props.formValue} | |
/> | |
<PrimaryButton | |
onClick={event => { | |
this.props.onSignUpClicked( | |
type, | |
this.signupForm, | |
this.state.recaptchaToken, | |
this.refreshToken | |
); | |
event.preventDefault(); | |
}} | |
> | |
{i18n.t('label.signUp')} | |
</PrimaryButton> | |
</form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in container file we are using ref to access the tcombform ref.
I just remove that ref and added tcombform ref in callBack function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then I assume this patch should be the template for doing it everywhere else now.
@@ -20,6 +20,7 @@ export default class Target extends Component { | |||
this.state = { | |||
label: countTarget !== 0 ? i18n.t('label.update') : i18n.t('label.save') | |||
}; | |||
this.formRef=React.createRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same question for the native apps implementation as for the web client. Can you give me an example implementation I can use everywhere for the native apps? What is the difference of this patch against
treecounter-app/app/components/Authentication/SignUp/SignUp.native.js
Lines 44 to 50 in 4ab1b4d
onSignUpClicked = type => { | |
if (this.signupForm.getValue()) { | |
Keyboard.dismiss(); | |
} | |
// eslint-disable-next-line no-underscore-dangle | |
this.props.onSignUpClicked(type, this.signupForm); | |
}; |
treecounter-app/app/components/Authentication/SignUp/SignUp.native.js
Lines 84 to 91 in 4ab1b4d
<Form | |
ref={ref => (this.signupForm = ref)} | |
type={signupFormSchema[!ProfileTypeParam ? Profiletype : type]} | |
options={ | |
this.props.schemaOptions[!ProfileTypeParam ? Profiletype : type] | |
} | |
value={this.props.formValue} | |
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are using TCombForm therefor it will only allow to access the method using ref. if we need to optimise this then we need to change TCombForm to Formik form that we are using in tree registration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactoring work to change all remaining old Tcomb forms to Formik is also still open in #2475 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give a tip how one can reproduce the bug within the local development environment (web client or native apps)?
@@ -37,14 +38,14 @@ export default class Target extends Component { | |||
style={{ flex: 1, marginTop: Platform.OS === 'ios' ? 140 : 100 }} | |||
> | |||
<Form | |||
ref={'setTargetForm'} | |||
ref={this.formRef} | |||
type={targetFormSchema} | |||
options={this.props.schemaOptions} | |||
value={this.props.treecounter} | |||
/> | |||
</CardLayout> | |||
<PrimaryButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. this is one example of a form where the ActivityIndicator within the PrimaryButton is not yet integrated (compare #1837), so the user does not know if the API requests for updating his target is still working or the app is crashed ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it works for me.
Actually I am also not able to reproduce this, but issue description I made some changes. |
@sanjayradadiya if you mark this draft as ready for review I could approve it. |
4eb50bb
to
c77495b
Compare
c77495b
to
6b3af40
Compare
won't be done any more as project will be deprecated |
Fixes #2216
Changes in this pull request: