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

Add profile share. #252

Closed
wants to merge 11 commits into from
Closed

Add profile share. #252

wants to merge 11 commits into from

Conversation

Kurogoma4D
Copy link
Contributor

Description

  • Show profile QR code.
  • Allow scan QR code for profile.
    • Show others profile from QR code.

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 self-assigned this Nov 6, 2023
@Kurogoma4D Kurogoma4D requested a review from a team as a code owner November 6, 2023 01:44
@Kurogoma4D
Copy link
Contributor Author

@FlutterKaigi/conference-app-2023
iOSでの実機デバッグが充分にできていないので、レビューの際に合わせてお願いしたいです 🙏

@@ -1,3 +1,4 @@
org.gradle.jvmargs=-Xmx1536M
android.useAndroidX=true
android.enableJetifier=true
dev.steenbakker.mobile_scanner.useUnbundled=true
Copy link
Contributor

Choose a reason for hiding this comment

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

pubspec.yaml Outdated
@@ -36,6 +36,7 @@ dependencies:

adaptive_dialog: ^1.9.0-0
auto_size_text: ^3.0.0
mobile_scanner: ^3.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

image_pickerの次の行に追加でどうでしょう。

Comment on lines +25 to +28
@override
Widget build(BuildContext context, GoRouterState state) {
return const ScanCodePage();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@override
Page<void> buildPage(BuildContext context, GoRouterState state) {
  return MaterialPage(
    key: state.pageKey,
    child: const ScanCodePage(),
    fullscreenDialog: true,
  );
}

こちらのコードに書き換えて、iOSだとmodalとして下から開くようになります。
また、自動的にBackButtonCloseButtonに置き換わります。こちらの方が意図通りかなと。

TargetPlatform.android || TargetPlatform.iOS => true,
_ => false,
};
final canReadQrCode = id != null && name.isNotEmpty;
Copy link
Contributor

Choose a reason for hiding this comment

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

nameの入力後、キーボード上で done をしないと、値が切り替わらない状態になっていました。
onChange をある程度監視するか、submit的なボタンを用意した方が良さそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

これはある程度意図的でした。onChange だと文字を入力するたびにQRコードが変化したりセクション自体が消えたりするのでUIに変化がありすぎるかなと思ってます。

} else {
final imageUrl = ref.watch(imageDownloadUrlProvider(url));
image = switch (imageUrl) {
AsyncData(value: final value) => value.isEmpty
Copy link
Contributor

Choose a reason for hiding this comment

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

isNotEmptyの方が自然な気がします。

@koji-1009
Copy link
Contributor

Profile画面でQRを出した際、スクロール時に若干フレームが落ちてますね。Androidだと注意深く見ると気になる程度、iOSだとスクロールのfpsが足りてないのがぱっと見でわかる程度です。なんでだろう。

Copy link

github-actions bot commented Nov 6, 2023

Commented by GitHub Bot

PR: #252
preview link

@Kurogoma4D Kurogoma4D closed this Nov 9, 2023
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.

None yet

2 participants