[Bug]: catalogのBuilderAddonの挙動が想定通りでない

K9i-0 commented 4 months ago

これに関する既存の Issue はありますか？

[X] 既存の Issue はありませんでした

現在の動作

scaffoldFinder.hasFoundが常にfalseでScaffoldがある場合もScaffoldでラップされている https://github.com/yumemi-inc/flutter-mobile-project-template/blob/main/apps/catalog/lib/main.dart#L57-L65

期待される動作

No response

環境

- Commit Hash: 0153501e8487f388e99243d36f56509ea98f9def
- App Version: 1.0.0
- Platform: iOS 17.2.1, Android 14 API level 34

blendthink commented 3 weeks ago

@mj-hd 作業時間確保できないかと思いますので、アサインから削除させていただきました 🙏

nerd0geek1 commented 2 weeks ago

@K9i-0 @blendthink Widgetbookに詳しくないのですが、findをflutter_testからimportしているのは良いのでしょうか？

K9i-0 commented 2 weeks ago

@nerd0geek1 findがおそらくtest中でしか機能しなくて、このIssueのような問題に繋がってると思ってます 🙏

nerd0geek1 commented 2 weeks ago

@K9i-0 意図としては以下の通りで良いでしょうか？

buildメソッドが以下のようにScaffoldを含む > Scaffoldでラップしない

@override
Widget build(BuildContext context, WidgetRef ref) {
  return Scaffold(
    body: Container(),
  );
}

buildメソッド内にScaffoldを含まない > Scaffoldでラップする

K9i-0 commented 2 weeks ago

その認識です 🙇

@Yamasaki-pan961 実装やまぱんさんだと思うのでメンション 🙏

Yamasaki-pan961 commented 2 weeks ago

@nerd0geek1

意図としては以下の通りで良いでしょうか？ buildメソッドが以下のようにScaffoldを含む > Scaffoldでラップしない buildメソッド内にScaffoldを含まない > Scaffoldでラップする

実装時はそのように考えてました！

そもそも「buildメソッド内にScaffoldを含まない > Scaffoldでラップする」こと自体不要な気がしてきました

nerd0geek1 commented 2 weeks ago

@Yamasaki-pan961 ただし、ScaffoldをかませないとWidget Treeが以下のようになり

黒背景となってしまうため、何かをはさむようにしても良いとは思います。

その点で、選択肢はColorBoxなどありますが、Theme(light / dark)への追従を考えるとScaffoldで良いのかなと思いました。

Yamasaki-pan961 commented 2 weeks ago

@nerd0geek1 背景が真っ黒になるという理由でScaffoldつけるようにしたことを思い出しました。

以下のようにMaterialで囲むのはどうでしょうか？ Theme(light / dark)への追従もできそうです

        BuilderAddon(
          name: 'Material',
          builder: (context, child) {
            return Material(
              child: child,
            );
          },
        ),

blendthink commented 2 weeks ago

@nerd0geek1 背景が真っ黒になるという理由でScaffoldつけるようにしたことを思い出しました。

以下のようにMaterialで囲むのはどうでしょうか？ Theme(light / dark)への追従もできそうです

@Yamasaki-pan961 Material で囲ってしまうと、タップ領域やエフェクトが変化してしまうケースがあるため、おすすめできません 🙏

Yamasaki-pan961 commented 2 weeks ago

@blendthink ご意見ありがとうございます！

単純にColordBoxにするのがよさそうですかね？

BuilderAddon(
  name: 'BackgroundColor',
  builder: (context, child) {
    return ColoredBox(
      color: Theme.of(context).scaffoldBackgroundColor,
      child: child,
    );
  },
),

blendthink commented 2 weeks ago

@blendthink ご意見ありがとうございます！

単純にColordBoxにするのがよさそうですかね？
BuilderAddon(
  name: 'BackgroundColor',
  builder: (context, child) {
    return ColoredBox(
      color: Theme.of(context).scaffoldBackgroundColor,
      child: child,
    );
  },
),

@Yamasaki-pan961 Scaffold で囲むといろいろといい感じで調整してくれるので、できれば Scaffold で囲みたいですね〜

@nerd0geek1 Scaffold.of(context) で何かいい感じの判定できたらいいかもと思っていたのですが、試していただいてもいいでしょうか？ 🙏

nerd0geek1 commented 2 weeks ago

@Yamasaki-pan961 @blendthink そもそも論なんですが、条件分岐を行わずにScaffoldで囲ってしまう、ではだめですかね？ WidgetBookで表示しようと思う対象は画面というより、コンポーネントレベルかと思うので、無条件にScaffoldで囲ってしまうでも困ることはないのではないかと。

blendthink commented 2 weeks ago

@Yamasaki-pan961 @blendthink

そもそも論なんですが、条件分岐を行わずにScaffoldで囲ってしまう、ではだめですかね？

WidgetBookで表示しようと思う対象は画面というより、コンポーネントレベルかと思うので、

無条件にScaffoldで囲ってしまうでも困ることはないのではないかと。

@nerd0geek1 案件によっては画面も対象なのです🙏

nerd0geek1 commented 2 weeks ago

@blendthink なおほど。それでいうと、SafeArea部分も見直したほうが良いですかね？ (現状、すべてのWidgetをSafeAreaで囲ってしまうことになるため、デザインによっては不都合が生じそう。)

blendthink commented 2 weeks ago

@blendthink

なおほど。

それでいうと、SafeArea部分も見直したほうが良いですかね？

(現状、すべてのWidgetをSafeAreaで囲ってしまうことになるため、デザインによっては不都合が生じそう。)

おそらくそうだと思います！ただ、そこの問題は今のところ上がってないのでひとまずは Scaffold だけ対応でいいかなと思います👍

yumemi-inc / flutter-mobile-project-template