-
-
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
feat: Upgrade the Flutter version to 3.24 #5613
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.
Hi @g123k!
That new dart super
syntax is a bit disturbing at first. I guess we'll get used to it.
@@ -124,12 +124,6 @@ class _SearchPageState extends State<SearchPage> { | |||
child: searchHelper.value!.widget, | |||
), | |||
], | |||
onPopPage: (Route<dynamic> route, dynamic result) { |
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 guess we needed that code.
Removed that because of a new 3.24 syntax?
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.
onPopPage
is now deprecated and the new method doesn't return a value.
So we can't do the same thing anymore.
With my tests, it seems there are no regression, but I'm not fully sure (hence the PR in draft)
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 wouldn't care that much about this feature being dropped.
Looking back at the code, it's when you search for a term, then search further "in the whole world" - i.e. without the country filter. Then if you go back, do you go back to the country results or the initial search input page: that's the purpose of the dropped code.
That shouldn't prevent you from merging this PR. In the worst case scenario there'll be a new issue, for a minor feature.
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.
By using a WillPopScope2, we can't get the same behavior.
It's fixed in the latest commit
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.
lgtm
There are two changes in this migration:
super.xxx
onPopPage
is deprecated on theNavigator