-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
lib/view/launch_view.dart
Outdated
| } | ||
|
|
||
| class _LaunchViewState extends State<LaunchView> { | ||
| void _toWeatherView() { |
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.
再帰的に実装してCloseボタン押下後にWeatherViewへ遷移するようにしました。🙋♂️
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.
[感想]
ばっちり実装できています 🎉
| import 'package:flutter_training/view/weather_view.dart'; | ||
| import 'package:go_router/go_router.dart'; | ||
|
|
||
| final router = GoRouter( |
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.
遷移先が増えていくにつれて見通しが悪くなるのでroutesの定義はroutesファイルに切り出しました。🙋♂️
lib/view/launch_view.dart
Outdated
|
|
||
| class _LaunchViewState extends State<LaunchView> { | ||
| void _toWeatherView() { | ||
| Future.delayed(const Duration(milliseconds: 500), () { |
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.
Durationの引数seconds は int 型で「0.5」の指定ができなかったので、milliseconds で「500」を指定して 0.5 秒にしています。🙋♂️
|
Ready for review 🚀 |
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.
@morikann
コメントしましたのでご確認お願いします!
| flutter: | ||
| sdk: flutter | ||
| flutter_svg: ^2.0.4 | ||
| go_router: ^6.5.0 |
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.
[感想]
今流行りのパッケージを使用していてチャレンジングでいいですね〜✨
lib/routes.dart
Outdated
| builder: (context, state) => const LaunchView(), | ||
| routes: [ | ||
| GoRoute( | ||
| path: 'weather_view', |
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.
[nits]
このような path に指定している文字リテラルは定数に切り出して、使いまわせるようにするか、 go_router_builder を使って型安全に遷移できるようにするなどするとより安全に開発進められ良さそうです!
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.
なるほど、確かにそうですね!
ご指摘ありがとうございます!
現在 go_router_builder を使っているのですが、自動生成された push メソッドの返り値の型が Future<void> -> void に変換されており、画面遷移から戻ってきたら処理を実行するというのができなさそうなのですが、この解決策があればご教示いただきたいです🙇♂️
extension $WeatherRouteExtension on WeatherRoute {
static WeatherRoute _fromState(GoRouterState state) => const WeatherRoute();
String get location => GoRouteData.$location(
'/weather_view',
);
void go(BuildContext context) => context.go(location);
// Future<void> から void に変換されている
void push(BuildContext context) => context.push(location);
void pushReplacement(BuildContext context) =>
context.pushReplacement(location);
}
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.
上記解決策がわからなかったため、path を定数に切り出して対応しました。🙋♂️
e39fee3
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.
現在 go_router_builder を使っているのですが、自動生成された push メソッドの返り値の型が Future -> void に変換されており、画面遷移から戻ってきたら処理を実行するというのができなさそうなのですが、この解決策があればご教示いただきたいです🙇♂️
ああ、、これは go_router_builder まだ対応していないのですね、、
Flutterチーム管轄になってから、 go_router の開発の進め方が悪くなっちゃったんですよね。。
いったん、定数に切り出しだけでいいかなと思います!
lib/view/launch_view.dart
Outdated
| } | ||
|
|
||
| class _LaunchViewState extends State<LaunchView> { | ||
| void _toWeatherView() { |
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.
[感想]
ばっちり実装できています 🎉
lib/view/launch_view.dart
Outdated
| void _toWeatherView() { | ||
| Future.delayed(const Duration(milliseconds: 500), () { | ||
| context.push('/weather_view').then((_) { | ||
| _toWeatherView(); |
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.
[感想]
go_router だと、 iOS のスワイプで戻ってきた時に、ここの処理が実行されないようでした。
修正は不要ですが、実案件ではこの辺りも気をつけながら実装進めていただければと思います 💪
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-03-29.at.00.59.23.mp4
push で戻り値が返ってくるようになったのがここ最近だったので、おそらくまだ対応されていない気がしています。。
↓対応されたプルリクエスト
flutter/packages#3368
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.
これは気づきませんでした...。ご教示いただきありがとうございます!
実案件では気をつけるようにします!!
lib/view/launch_view.dart
Outdated
| class _LaunchViewState extends State<LaunchView> { | ||
| void _toWeatherView() { | ||
| Future.delayed(const Duration(milliseconds: 500), () { | ||
| context.push('/weather_view').then((_) { |
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.
[nits]
mounted のチェックをしておかないと、クラッシュする危険性があるため注意が必要です。
if (mounted) {
context.push('/weather_view').then((_) {
_toWeatherView();
});
}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.
DON'T use BuildContext across asynchronous gaps. という警告がなかったため気づきませんでした... 。
ご指摘ありがとうございます!
ついでに書き方も then から async/await を使った書き方に変更しました。🙋♂️
|
@blendthink |
blendthink
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.
|
レビューありがとうございます! |
課題
close #4
対応箇所
動作
actual.mp4