-
-
Notifications
You must be signed in to change notification settings - Fork 280
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
fix: Autofocus confirm password textfield on password field submission. #1288
fix: Autofocus confirm password textfield on password field submission. #1288
Conversation
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.
Thanks for your PR @CyberWake.
However I think a generic solution would be better here.
Instead of doing the focus trick on the password field only, it would be better to:
- Add a
nextFocus
optional field to SmoothTextFormField - If it's provided, do your call in the
onFieldSubmitted
), | ||
const SizedBox(height: space), | ||
SmoothTextFormField( | ||
focusNode: confirmPassword, | ||
type: TextFieldTypes.PASSWORD, | ||
controller: _password2Controller, | ||
textInputAction: TextInputAction.next, |
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.
I'm sure your way also works, but have you tried setting the textInputAction for the last field to TextInputAction.done
, besides of that I can't think of why the focus move doesn't work for the last one
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.
So setting the textInputAction
to TextInputAction.done
for the confirm password field won't work I guess since the issue starts with the internal mechanism of flutter I guess since the control doesn't get passed automatically ideally the focus passes to the next field inside the for without use of focusNode
or textInputAction
. But sometimes this doesn't happen due to some reasons like use of obscure field etc. Then in that case we explicitly need to use this focusNode mechanism.
Surely I try this one since this reduces passing of one argument. Good suggestion hope this works. 🤞🏻 |
I have an input from my side to stand the current implementation. Like in my case I have added the condition if the confirm password is not same as password then only focus to confirm password field this custom logic cannot be added if we implement that common way of using Secondly in nextFocus methodology we'll still need to pass the focus for the current field like for confirm password SmoothTextFormField we'll need to pass the focus to uniquely identify the confirm password fields focus. So the reduction of argument won't we a case which I just got. Let me know you input on this. |
@M123-dev could you please update me on this PR. |
I guess you can my solution can easily be implemented.
|
@g123k the solution suggested still need the two argument one the |
// Checks if confirm password is equal to password if not then | ||
// move the focus to the confirm password field | ||
if (_password2Controller.text != value) { | ||
FocusScope.of(context).requestFocus(confirmPassword); | ||
} |
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.
Then lets leave it like that, just two small things to sum up
- I could imagine it beeing a bit confusing if sometimes you jump to the second password and another time two steps further. Its certainly great when you know it, but when not not
- Please add a comment here and where you create the Focus node that there was a bug and thats why we have this hacky way for changing the focus only for one field
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.
@M123-dev sure I'll do the points marked out.
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.
Any update @CyberWake
merge conflicts on this one |
I think we should work on #2555 and dismiss this PR |
ok, closing 👍 |
What
Screenshot
Fixes bug(s)