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

Implements sessions screen. #165

Merged
merged 10 commits into from
Oct 3, 2023
Merged

Implements sessions screen. #165

merged 10 commits into from
Oct 3, 2023

Conversation

Kurogoma4D
Copy link
Contributor

Description

Fixes #17

Type of change

Please delete options that are not relevant.

  • New feature
  • Bug fix
  • Refactor
  • Style
  • Documentation
  • CI/CD
  • Other

How Has This Been Tested?

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.

Checklist

  • My code follows the style guidelines of this project.
  • My code has been formatted with flutter format lib.
  • My code has been fixed with dart fix --apply lib.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

@Kurogoma4D Kurogoma4D requested a review from a team as a code owner October 1, 2023 09:02
@Kurogoma4D Kurogoma4D self-assigned this Oct 1, 2023
}

extension LocaleTextEx on LocaleText {
String get(Locale locale) => switch (locale.languageCode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

lib/model/sessions/session_provider.dart Outdated Show resolved Hide resolved

return SelectionContainer.disabled(
child: ListView(
padding: const EdgeInsets.symmetric(vertical: 16),
Copy link
Contributor

Choose a reason for hiding this comment

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

dartfmtが現時点で対応できていないので、個別にチェックになってしまうのですが、trailing commaを入れてもらっても良いでしょうか 🙏
ref dart-lang/dart_style#1253

また、 horizontalcontext.spacing を入れてもらえると、他のページと揃ったmarginがある程度入るようになるかなと思います。

],
onSelectionChanged: (rooms) {
if (rooms.isEmpty) return;
ref.read(selectedRoomProvider.notifier).update(rooms.first);
Copy link
Contributor

Choose a reason for hiding this comment

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

ページ内に閉じる状態管理であれば、ConsumerStatefulWidgetで対応してもいいかもしれないです 🤔
この辺りは、tabの状態をURLと紐づけるかどうかで検討していただければ、と言う感じです! (仕様を考えましたが、SegmentedButtonで管理する場合には、どちらでもいいかなと思いました)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

それで言うと、むしろURLベースにして ?room=1 とかクエリパラメータで飛べるようにするのもありかなと思ったんですがどうですか?

Copy link
Contributor

Choose a reason for hiding this comment

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

ありだと思います!

}

class _SessionSection extends ConsumerWidget {
const _SessionSection({super.key, required this.session});
Copy link
Contributor

Choose a reason for hiding this comment

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

半ばanalyzerのご検知のようなのですが、未使用で参照していないことは明らか(private classのため)なので、super.keyは削除で良さそうに思います。

_ => null,
};

return Card(
Copy link
Contributor

Choose a reason for hiding this comment

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

outline style 👍

Copy link
Contributor

@koji-1009 koji-1009 left a comment

Choose a reason for hiding this comment

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

LGTM!
よければ、suggestをadd suggestion to batch でまとめて追加しておいていただけますと 🙏

lib/ui/router/branch/router_app_sessions.dart Outdated Show resolved Hide resolved
lib/ui/router/branch/router_app_sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
lib/ui/screen/sessions/sessions.dart Outdated Show resolved Hide resolved
Co-authored-by: Koji Wakamiya <koji.wakamiya@gmail.com>
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Commented by GitHub Bot

PR: #165
preview link

@Kurogoma4D Kurogoma4D merged commit 1dde0d5 into main Oct 3, 2023
3 checks passed
@Kurogoma4D Kurogoma4D deleted the feature/sessions-screen branch October 3, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sessions一覧画面の作成
2 participants