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

NFC loading and storing implementation for Achievement Rally #1043

Merged
merged 39 commits into from
Sep 6, 2023

Conversation

momomomo111
Copy link
Contributor

@momomomo111 momomomo111 commented Sep 1, 2023

Issue

Overview (Required)

  • The NFC has been configured to trigger animations upon reading, and the results are saved locally.
  • Commands for verifying NFC
    • $ adb shell am start -W -a android.nfc.action.NDEF_DISCOVERED -d "https://confsched2023.page.link/e" io.github.droidkaigi.confsched2023.dev
  • A ResolveDynamicLinksActivity is being added to resolve DynamicLink's DeepLink, and a CompleteAchievementActivity to trigger animations and save them locally.
    The Compose part of the CompleteAchievementActivity has been added to a module called Animation.

Movie (Optional)

demo.mp4

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Test Results

209 tests   209 ✔️  8m 50s ⏱️
  11 suites      0 💤
  11 files        0

Results for commit b0c441a.

♻️ This comment has been updated with latest results.

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 1, 2023 01:44 Inactive
@DroidKaigi DroidKaigi deleted a comment from github-actions bot Sep 3, 2023
# Conflicts:
#	app-android/src/main/java/io/github/droidkaigi/confsched2023/KaigiApp.kt
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 3, 2023 20:53 Inactive
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

Snapshot diff report

File name Image
AchievementsScreenTe
st.checkLaunchShot_2
_compare.png

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 4, 2023 05:45 Inactive
@momomomo111 momomomo111 changed the title Functions from NFC are being implemented. NFC loading and storing implementation for Achievement Rally Sep 4, 2023
@momomomo111 momomomo111 marked this pull request as ready for review September 4, 2023 07:10
@momomomo111 momomomo111 requested a review from a team as a code owner September 4, 2023 07:10
Comment on lines 82 to 94
if (uiState.isResetButtonEnabled) {
item(
key = "reset_button",
span = { GridItemSpan(SingleItemSpanCount) },
) {
Button(
modifier = Modifier.fillMaxWidth(),
onClick = onReset,
) {
Text(text = "Reset")
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This button is no longer included in the version of the release

Comment on lines 33 to 35
val intent = Intent(this, MainActivity::class.java)
startActivity(intent)
finish()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If NFC reading fails, the default screen will be activated

Copy link
Contributor

@jmatsu jmatsu left a comment

Choose a reason for hiding this comment

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

Could you please have a look? Questions are welcome.


fun onReachAnimationEnd() {
viewModelScope.launch {
achievementRepository.saveAchievements(animationLottieRawResStateFlow.value.first)
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be dangerous. This is because the app finishes just after viewModelScope.launch(). So this block can be canceled.
I recommend saving this in onReadDeeplinkHash()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the following commit🙏

ed1457c

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 5, 2023 08:51 Inactive
@takahirom
Copy link
Member

@momomomo111
I've refactored some classes. Please check it out 🙏
https://github.com/DroidKaigi/conference-app-2023/pull/1043/files/acd3b622d78081008692f82a58bdcda1d520367b..88f53071af1f108387c7903b07094c209f349904

)
}
if (uiState.isResetButtonEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we merge this implementation?

Copy link
Member

Choose a reason for hiding this comment

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

@momomomo111 I'm wondering if this button will be released.

@takahirom
Copy link
Member

I don't check if this works and we don't have any tests for this. Please check it by yourself 🙇

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

Hi @takahirom! Codes seem to be unformatted. To resolve this issue, please run ./gradlew detekt --auto-correct and fix the results of ./gradlew lintDebug.. Thank you for your contribution.

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 5, 2023 15:23 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 5, 2023 20:39 Inactive
@momomomo111
Copy link
Contributor Author

momomomo111 commented Sep 5, 2023

I don't check if this works and we don't have any tests for this. Please check it by yourself 🙇

I confirmed that A through E actually work after testing them.🙏

Copy link
Member

@takahirom takahirom left a comment

Choose a reason for hiding this comment

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

Thanks for your effort!

@takahirom
Copy link
Member

I'm not sure if @jmatsu is comfortable with this PR. I believe we can address any issues post-merge. Please let @momomomo111 know if there are any concerns.

@takahirom takahirom merged commit 8757ee8 into main Sep 6, 2023
8 checks passed
@takahirom takahirom deleted the add-dynamic-links-nfc branch September 6, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read URL from NFC tag Apply changes to the Stamps screen
3 participants