Skip to content
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

ChangeNotifierの実装(メインページ) #16

Merged
merged 21 commits into from
Jul 15, 2020
Merged

Conversation

thoth000
Copy link
Collaborator

issue

#13

やったこと

  • pagesディレクトリ内でmainディレクトリを作成
  • mainディレクトリ内にhome talk profileディレクトリを作成しdartファイルを分ける
  • ページごとにChangeNotifierを設計

Copy link

@ho2ri2s ho2ri2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一気にController作られたんですね:eyes:
こんな感じで、Controllerの作り方自体に修正があった際に少し修正範囲が大きくなってしまうので、ミニマムでPRを出してもらえると楽かもしれません!


class HomePage extends StatelessWidget {
@override
Widget build(BuildContext context) {
final _homeController = Provider.of<HomeController>(context);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build配下に listen: true でProvideすると、HomeControllerに変更があるたびに、このcontext(HomePage)が丸ごと再描画されてしまうので、あんまり良くないかもです!
ここでは

  • listen: false で定義しておいて、単発呼び出しではこれを使ってあげる
  • 変更を監視したいプロパティの場合はちょっとめんどうくさいけど、 Provider.of<HomeController>(context, listen: true).hoge みたいに逐一取得してあげる感じですかね

前に記事書いてみたので参考にしてみてください!
https://qiita.com/hohohoris/items/8aecf6a99b278d67d13b

retrun  ChangeNotifierProvider<HomeController>(
      create: (_) => HomeController(),
      child: SingleChildScrollView(
         // 略
    )

とすると `main.dart` でMultiProviderしなくてもpageごとにできると思います!

class HomeController with ChangeNotifier{
HomeController();

Future<User> getMeById(String userId) async {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

基本Controllerのメソッドは戻り値を Future<void> にして、 Controllerのプロパティで Userを持ってあげます。
するとFuture型を処理せずともUserという型でWidgetに通知できるからです。
具体的には、

Suggested change
Future<User> getMeById(String userId) async {
User user;
Future<void> getMeById(String userId) async {
await Future.delayed(Duration(seconds: 1));
user = User(name: "test", imgUrl: "https://dot.asahi.com/S2000/upload/2019100100055_1.jpg");
notifyListeners()

Copy link

@ryu1sazae ryu1sazae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

恐縮ながら 🙇
間違ってたり、もうこれで決まってるからってとこあったら遠慮なく言ってください!

@override
Widget build(BuildContext context) {
final _mainController = Provider.of<MainController>(context);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

メソッド内で定義されたものはそもそもメソッド内のスコープでしか使用できないのでprivateにする必要はなさそうですね 👌

);
}

Widget bottomNavigation() {
Widget bottomNavigation(BuildContext context) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

逆にここは他のファイルから参照されていなければprivateにして良さそうです 👌

);
}

Widget bottomNavigation() {
Widget bottomNavigation(BuildContext context) {
final _mainController = Provider.of<MainController>(context);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

上に同じくprivateにする必要はなさそうですね 👌

@@ -10,8 +12,9 @@ class TalkPage extends StatefulWidget {
class _TalkPageState extends State<TalkPage> {
@override
Widget build(BuildContext context) {
final _talkController = Provider.of<TalkController>(context);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

privateにする必要はなさそうですね 👌
他にも同様の場所がありそうなので確認して頂けると 🙇

Comment on lines 38 to 40
return Center(
child: Text("該当するユーザーがいません"),
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Center(
child: Text("該当するユーザーがいません"),
);
return const Center(
child: Text("該当するユーザーがいません"),
);

のように書けますかね?
constをつけると、コンパイル時に確定できるので、パフォーマンスが上がります
以下参考

Use const widgets where possible. (This is equivalent to caching a widget and re-using it.)

https://medium.com/flutter-jp/state-performance-7a5f67d62edd

@@ -209,3 +75,102 @@ class HomePage extends StatelessWidget {
);
}
}

class MyTile extends StatelessWidget {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

少しfileが必要以上に肥大化しいてるように思えるので、個別のWidgetは /Widgets/を作ってその中に入れても良さそうですね 🙆‍♂️
このままなら

Suggested change
class MyTile extends StatelessWidget {
class _MyTile extends StatelessWidget {

とできそうでしょうか?

);
}

HomePage({Key key}) : super(key:key);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これってないとコンパイルできませんっけ? 🤔

@@ -7,7 +7,7 @@ import 'package:chat_flutter/config/app_text_size.dart';
import 'package:provider/provider.dart';

class HomePage extends StatelessWidget {
HomePage({Key key}) : super(key:key);
HomePage({Key key}) : super(key: key);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

別の箇所で言及しましたが、ここがなくても良いって箇所が他にもありそうですね 🙇

Comment on lines 16 to 18
user = User(
name: "test",
imgUrl: "https://dot.asahi.com/S2000/upload/2019100100055_1.jpg");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
user = User(
name: "test",
imgUrl: "https://dot.asahi.com/S2000/upload/2019100100055_1.jpg");
user = User(
name: "test",
imgUrl: "https://dot.asahi.com/S2000/upload/2019100100055_1.jpg",
);

の方が自然ですかね?

currentIndex: _controller.currentIndex,
items: [
BottomNavigationBarItem(
icon: Icon(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

別箇所でも言及しましたが、const付けれそうですね 👌
https://github.com/flutter/flutter/blob/master/analysis_options.yaml
ここら辺の導入に関してもうまく決めれると良さそうですね 🙆‍♂️

@ryu1sazae
Copy link

コメントして気づきましたが、squashしてあげると別のコミットでの変更の上書きが無くなって良さそうですね 🙆‍♂️
参考

Copy link

@ryu1sazae ryu1sazae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

squashに関して
きっと @ho2ri2s あたりが詳しいと思うのでmtgのときに聞いてみてください 👍

constの付け忘れや、statelessWidgetの別ファイルへの切り出しなど、まだ改善の余地はありそうですがそこはまた後で決めるって感じでもいいかもですね(本当はお勧めしませんが) 🙆‍♂️

お疲れ様です 👍

@ryu1sazae
Copy link

#18 がマスターにマージされたら
git merge --no-ff
でマージした後修正加えてpushお願いします! 🙇

),
ListView.builder(
physics: ScrollPhysics(),
scrollDirection: Axis.vertical,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://api.flutter.dev/flutter/widgets/ListView/ListView.builder.html
今更で申し訳ないんですけどここは defaultがAxis.verticalなのでいらなさそうですね 🆗

Suggested change
scrollDirection: Axis.vertical,

),
ListView.builder(
physics: ScrollPhysics(),
scrollDirection: Axis.vertical,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@ho2ri2s ho2ri2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

お疲れ様でした👏

Copy link

@ryu1sazae ryu1sazae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

色々とありがとうございました 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants