-
-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: dismiss keyboard on tap in search page (iOS) #2891
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
base: master
Are you sure you want to change the base?
Conversation
|
Hello, just checking in on this PR. I understand you might be busy, but I’d appreciate it if you could take a look when you have a moment. I'm happy to resolve any conflicts or make changes if needed. |
|
I understand you must be busy, but I would highly appreciate it if you could spare some time to review this PR. As a part of a university course assignment, feedback or a merge is crucial for my submission deadline. Please let me know if there are any issues I need to resolve, or if you have specific feedback on the code. Thank you for your time! |
Alastair-L
left a comment
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.
Hi there, thanks for your contribution :).
Positives:
- You did a good job with the scope of the issue and fix
- Thank you for contributing at all!
Improvements:
- I would ask that if you use AI to generate the code that you should disclose this.
- Make sure not to accidentally include extra files in the PR.
- Don't create a PR against the master branch (in future please read the contribution guidelines file found in most open source projects, here: https://github.com/KRTirtho/spotube/blob/master/CONTRIBUTION.md)
- Usually, it's best to include more information about the issue before submitting a fix, and creating a github issue with (ideally) screenshots/videos.
Note that the method in this PR may not be best way to achieve the overall goal. For example this may be a little cleaner and avoids a level of nesting:
FYI I'm not a native flutter dev.
| @@ -0,0 +1,11 @@ | |||
| { | |||
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 don't think it's needed or desired to commit this file. Try adding it to .gitignore instead, or stage a subset of files before committing.
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.
Remove this file as well.
| _ => const SearchPageAllTab(), | ||
| }, | ||
| child: GestureDetector( | ||
| onTap: () => FocusScope.of(context).unfocus(), |
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.
Seems like this is not the correct way to lose focus in modern Flutter:
https://stackoverflow.com/questions/44991968/how-can-i-dismiss-the-on-screen-keyboard
https://stackoverflow.com/questions/73215495/what-is-the-difference-between-focusscope-ofcontext-unfocus-and-focusmana
| }, | ||
| child: GestureDetector( | ||
| onTap: () => FocusScope.of(context).unfocus(), | ||
| behavior: HitTestBehavior.translucent, |
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 don't think translucent is what we want here (I think the default is correct). See the options here:
https://api.flutter.dev/flutter/rendering/HitTestBehavior.html
Summary
Fix the issue where iPhone users cannot dismiss the keyboard after searching.
Problem
Solution
GestureDetectorHitTestBehavior.translucentto detect taps on empty spaceFocusScope.of(context).unfocus()on tap to dismiss keyboardChanges
lib/pages/search/search.dart: Wrap search results area withGestureDetectorto dismiss keyboard on tapTest plan