Issue 173 ページング機能のエラー発生時の復帰処理を実装 #209

Aosanori · 2024-05-21T09:09:52Z

概要

close: [Feature]: ページング機能のエラー発生時の復帰処理を実装する #173
ページング機能 #2 にて基礎的なページング機能は実装されていましたが、2ページ目以降にエラーが発生した場合の復帰処理は未実装でしたので、2ページ目以降にエラーが発生した場合の復帰処理を実装いたしました。

レビュー観点

@akatsuki174 さんのお陰で解決しました！ありがとうございました！
~~common_paging_view.dartにおいて、~~
~~2ページ目以降の読み込み、表示、再読み込みの3つについての条件分岐が複雑であり、~~
~~三項演算子では可読性に乏しくなると考えました。~~
~~そこで無理矢理ですが、可読性の観点からswitch文を用いました。~~

~~こちらについてさらに良い方法がございましたら、ご教授いただけますと幸いです~~

switch (data.hasMore) {
  true when hasError && isLoading => const Center(
      child: Padding(
        padding: EdgeInsets.all(16),
        child: CircularProgressIndicator(),
      ),
    ),
  true when hasError && !isLoading => _EndItemWhenError(
      onPressed: () async =>
          ref.read(_provider.notifier).loadNext(),
    ),
  true when !hasError => _EndItem(
      onScrollEnd: () async =>
          ref.read(_provider.notifier).loadNext(),
    ),
  true => null,
  false => null,
},

レビューレベル

Lv1: ぱっとみて違和感がないかチェックして Approve する
Lv2: 仕様レベルまで理解して、仕様通りに動くかある程度検証して Approve する
Lv3: 実際に環境で動作確認したうえで Approve する

レビュー優先度

すぐに見てもらいたい ( hotfix など ) 🚀
今日中に見てもらいたい 🚗
今日〜明日中で見てもらいたい 🚶
数日以内で見てもらいたい 🐢

画像 / 動画

読み込み失敗時に再読み込みのためのボタンを設置いたしました。
ボタンをタップ時に、再読み込みが行われます。その際に読み込み中はCircularProgressIndicatorが表示されるようにしました。

Before	After	Design
2024-05-21.18.15.24.mov	2024-05-21.17.34.39.mov

動作確認手順

初回読み込みのために最初は通常通りにアプリ起動
2ページ目以降は機内モードに設定もしくはPCのWIFIを切ってから、スクロールし追加読み込みを行う。
読み込みが失敗したことを確認したら、再びシミュレータをオンラインの状態に戻しrefreshボタンをタップする。
正常に次のページが読み込まれたら動作確認完了。

備考

github-actions · 2024-05-21T09:10:32Z

Ready for review 🚀

github-actions · 2024-05-21T09:12:07Z

Visit the preview URL for this PR (updated for commit d09e747):

https://flutter-mobile-project-template-catalog--pr209-issue-1-j5t2jn9g.web.app

_{(expires Mon, 24 Jun 2024 08:14:17 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 9ea56735a63d07a7cfe62eb204b0528284c37c23}

akatsuki174

コメントしましたが、Flutter/Dartからしばらく離れているので、他の方のレビューもいただけるといいかなという状態です 🙇

packages/cores/core/lib/src/ui/util/common_paging_view.dart

Aosanori · 2024-05-27T07:07:17Z

愚問で申し訳ござませんが、CIがcheck-format-ci.sh で失敗している原因を教えていただきたいです。

Switch式を切り出す以前のテスト(c82f97a)は成功していましたが、Switch式の切り出し等のリファクタリングを行った結果、CIからエラーが出力されるようになりました。(1a3cf4c),(bc7299b)

そこでテストが失敗している原因の究明のためにローカルで実行した結果、以下の出力が得られました。

$ bash scripts/check-format-ci.sh

Fixes might be needed for ./packages/cores/core/.dart_tool/build/entrypoint/build.dart
Fixes might be needed for ./apps/app/.dart_tool/flutter_build/dart_plugin_registrant.dart
Fixes might be needed for ./apps/app/.dart_tool/build/entrypoint/build.dart

check-format-ci.shでは、コードが警告の有無、正しく整形されているかをチェックするものと認識しております。しかしながらCIのエラーの有無に関わらず出力は変化しないため、原因の見当がつきません。

お忙しいところ恐縮ですが、回答の方お待ちしております。

K9i-0 · 2024-05-30T02:36:17Z

@Aosanori

[cores_core]: Changed ./lib/src/ui/util/common_paging_view.dart

このファイルのフォーマットが必要みたいですよ 🙆‍♂️

Aosanori · 2024-05-30T03:00:15Z

@K9i-0
お忙しい中ありがとうございます！
おかげで解決することができました！

warahiko

レビューが遅れて申し訳ないです
全体的な動きは問題なく、UX が向上していると感じました 👍

細かい点をいくつかコメントしているので、確認いただけるとありがたいです！

warahiko · 2024-06-13T07:01:46Z

packages/cores/core/lib/src/extension/async_value.dart

@@ -29,16 +34,17 @@ extension AsyncValueExtension<T> on AsyncValue<T> {
    bool skipErrorOnHasValue = false,
  }) {
    if (skipErrorOnHasValue) {
+      logger.shout(isLoading);


これは確認用のが残ってしまっている？

d2b6a70にて修正いたしました！

warahiko · 2024-06-13T07:34:45Z

packages/cores/core/lib/src/ui/util/common_paging_view.dart

@@ -64,6 +64,40 @@ class CommonPagingView<N extends PagingAsyncNotifier<D, T>,

  final void Function(AppException e) _onError;

+  Widget _endItem(


ここは nullable にして、空を返すケースは null にしてもいいかもですね
「なにも表示しない」を明確化できそうです

01fc576にて修正いたしました！

warahiko · 2024-06-13T07:46:37Z

packages/cores/core/lib/src/ui/util/common_paging_view.dart

+      );
+    }
+
+    if (!hasError) {


ここで残りのケース全て受けているので、 L.98 には入らないですかね
単純に return としてもよさそうですし、「エラーがないとき」を明示したければ L.98 で throw するようにしてもよさそうです

@warahiko
ご指摘ありがとうございます！
ちなみL.98に入らないのであれば、if(!hasError)を取り払っても問題ないですか？
そのようにすれば返り値にnullの可能性が消失するのでそちらの方が良いのかなと考えましたが意見を賜りたいです！

ちなみに上でご指摘いただいた「なにも表示しない」を明確化するにはSizedBox.shrink();等の空のWidgetを返すのではなく基本的にはnullで返したほうが良いのでしょうか？
今後の開発の参考にさせて頂きたいのでご回答いただけますと幸いです！

L.98に入らないのであれば、if(!hasError)を取り払っても問題ないですか？

はい、自分は問題ないと思います！

「なにも表示しない」を明確化するにはSizedBox.shrink();等の空のWidgetを返すのではなく基本的にはnullで返したほうが良いのでしょうか？

今回のケースであれば、この API 自体が null で返す仕様になっているのでそうしたほうがよさそうですね
一般的にも、たとえサイズゼロの要素でも処理が必要ですし、 min(Height|Width) があればそれが適用されたりする（しますよね......？ちゃんと調べられてはないですが......）ので、できるのであればそもそも要素として入れない、というのは優先すべきじゃないかなと思いました
一方で普通の StatelessWidget#build() などは non-null な Widget を返す必要があるので、こういうときには仕方ないかな、くらいかなと

（などと書いていますが初心者の一意見なので、もっと使い慣れている人に聞いたほうがよさそうです）

返信ありがとうございます！
確かにこの API 自体が null で返す仕様になっているので、nullとして要素を存在させない方が自然な感じはありますね！ありがとうございます！

65e8d62にて修正いたしました！

warahiko · 2024-06-13T07:47:56Z

packages/cores/core/lib/src/ui/util/common_paging_view.dart

+      return const Center(
+        child: Padding(
+          padding: EdgeInsets.all(16),
+          child: CircularProgressIndicator(),
+        ),
+      );


他の EndItem と粒度は同じですがここだけ直書き、というのに少し違和感を感じました
_EndItem との違いを分かりやすくするためにも、名前があるとよさそうに思います

605536dにて修正いたしました！

warahiko · 2024-06-13T07:51:11Z

packages/cores/core/lib/src/ui/util/common_paging_view.dart

-                // Displays EndItem to detect scroll end
-                // if more data is available and no errors.


ここのコメントが消えてしまっていますね
EndItem という naming だと少しここの意図は分かり辛いので残しておくほうがよさそうに思います
もしくは、もう一方の ProgressIndicator の naming との兼ね合いでいい感じに名前をつけてもよさそうです

018eb9fにて修正いたしました！

warahiko · 2024-06-13T07:52:02Z

packages/cores/core/lib/src/ui/util/common_paging_view.dart

@@ -64,6 +64,40 @@ class CommonPagingView<N extends PagingAsyncNotifier<D, T>,

  final void Function(AppException e) _onError;

+  Widget _endItem(


切り出し 👍

Aosanori self-assigned this May 21, 2024

yumemi-team-review-requester bot requested review from a team, akatsuki174 and warahiko and removed request for a team May 21, 2024 09:10

akatsuki174 reviewed May 27, 2024

View reviewed changes

packages/cores/core/lib/src/ui/util/common_paging_view.dart Outdated Show resolved Hide resolved

packages/cores/core/lib/src/ui/util/common_paging_view.dart Outdated Show resolved Hide resolved

Aosanori force-pushed the issue-173 branch from b676646 to b6f8a59 Compare May 30, 2024 02:55

mqkotoo mentioned this pull request Jun 3, 2024

[Feature]: ドキュメントファイルを日本語対応する part2 #218

Closed

1 task

warahiko reviewed Jun 13, 2024

View reviewed changes

Aosanori added 13 commits June 17, 2024 17:02

✨ 読み込みが失敗しても再トライできるようにloadNext()の条件分岐を変更

4c3416e

✨ 読み込みが失敗した時にrefreshボタンを表示するようなコンポーネントの作成

53cc0c7

✨ 再読み込み時に動的に読み込み表示から結果表示に切り替わるように

5946e8a

✨ 不要なコメントを削除

5fc64b3

♻️ EndItem部分をswitch式を関数とif文で切り出し

92705d5

♻️ 不要なimportの削除

2c506bc

🐛 欠落していた条件分岐の追加

1bfead6

♻️ Dart format

89f494c

不要なloggerを削除

d2b6a70

💡 誤って消したコメントを再度記入

018eb9f

♻️ 復帰時のローディングを表すWidgetを_ReloadingItemに改名

605536d

♻️ 全ての要素が読み込み終わった後に表示するWidgetをnullに

01fc576

♻️ 不要な条件分岐を排除

65e8d62

Aosanori force-pushed the issue-173 branch from b6f8a59 to 65e8d62 Compare June 17, 2024 08:02

github-actions bot added the @cores/core label Jun 17, 2024

🚨 Async_value.dartのunused_importを解消

d09e747

Aosanori requested a review from blendthink June 17, 2024 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 173 ページング機能のエラー発生時の復帰処理を実装 #209

Issue 173 ページング機能のエラー発生時の復帰処理を実装 #209

Aosanori commented May 21, 2024 •

edited

github-actions bot commented May 21, 2024

github-actions bot commented May 21, 2024 •

edited

akatsuki174 left a comment

Aosanori commented May 27, 2024 •

edited

K9i-0 commented May 30, 2024

Aosanori commented May 30, 2024

warahiko left a comment

warahiko Jun 13, 2024

Aosanori Jun 17, 2024

warahiko Jun 13, 2024

Aosanori Jun 17, 2024

warahiko Jun 13, 2024

Aosanori Jun 14, 2024

warahiko Jun 14, 2024

Aosanori Jun 14, 2024

Aosanori Jun 17, 2024

warahiko Jun 13, 2024

Aosanori Jun 17, 2024

warahiko Jun 13, 2024

Aosanori Jun 17, 2024

warahiko Jun 13, 2024

		@@ -64,6 +64,40 @@ class CommonPagingView<N extends PagingAsyncNotifier<D, T>,

		final void Function(AppException e) _onError;

		Widget _endItem(

		// Displays EndItem to detect scroll end
		// if more data is available and no errors.

Issue 173 ページング機能のエラー発生時の復帰処理を実装 #209

Are you sure you want to change the base?

Issue 173 ページング機能のエラー発生時の復帰処理を実装 #209

Conversation

Aosanori commented May 21, 2024 • edited

概要

レビュー観点

レビューレベル

レビュー優先度

画像 / 動画

動作確認手順

備考

github-actions bot commented May 21, 2024

github-actions bot commented May 21, 2024 • edited

akatsuki174 left a comment

Choose a reason for hiding this comment

Aosanori commented May 27, 2024 • edited

K9i-0 commented May 30, 2024

Aosanori commented May 30, 2024

warahiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aosanori commented May 21, 2024 •

edited

github-actions bot commented May 21, 2024 •

edited

Aosanori commented May 27, 2024 •

edited