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

[ABW-3435] - Display home cards carousel #1033

Merged
merged 18 commits into from
Jul 11, 2024

Conversation

sergiupuhalschi-rdx
Copy link
Contributor

@sergiupuhalschi-rdx sergiupuhalschi-rdx commented Jul 4, 2024

Description

This PR adds home cards carousel functionality.

How to test

The users from organic app installs use-case:

  1. Create a new wallet
  2. The 'Connector' card will appear on the home screen at the top

@sergiupuhalschi-rdx sergiupuhalschi-rdx marked this pull request as ready for review July 9, 2024 17:15
Copy link
Contributor

@micbakos-rdx micbakos-rdx left a comment

Choose a reason for hiding this comment

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

The carousel works as it is described. Couple of questions.

  1. I realised that we inform sargon to check what cards to show with the walletCreated callback. If so we lose one case. Now that we remove the hint to link to a new connector, what should happen to existing users that never connected to CE and update to this version? They will never see the card to connect to CE. Is this expected?
  2. The Continue on dApp in browser card, does nothing on click, you can only close it via the X button. I guess this is expected?

private fun bootstrapHomeCardsManager() {
scope.launch {
runCatching { homeCardsManager.bootstrap() }
.onFailure { Timber.d("HomeCardsManager init error: ${it.message}") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use w instead of d?


override suspend fun cardDismissed(card: HomeCard) {
runCatching { homeCardsManager.cardDismissed(card) }
.onFailure { Timber.d("Failed to dismiss home card. Error: $it") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the same here?


val isCloudBackupEnabled = args.requestSource == CreateAccountRequestSource.FirstTimeWithCloudBackupEnabled
changeBackupSettingUseCase(isChecked = isCloudBackupEnabled)
}
}

private suspend fun initHomeCards() {
runCatching { homeCardsManager.walletCreated() }
.onFailure { Timber.d("Failed to notify HomeCardsManager about wallet creation. Error: $it") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

applicationScope.launch {
withContext(defaultDispatcher) {
runCatching { homeCardsManager.deferredDeepLinkReceived(value) }
.onFailure { Timber.d("Failed to notify HomeCardsManager about deep link receiving. Error: $it") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@sergiupuhalschi-rdx
Copy link
Contributor Author

The carousel works as it is described. Couple of questions.

  1. I realised that we inform sargon to check what cards to show with the walletCreated callback. If so we lose one case. Now that we remove the hint to link to a new connector, what should happen to existing users that never connected to CE and update to this version? They will never see the card to connect to CE. Is this expected?
  2. The Continue on dApp in browser card, does nothing on click, you can only close it via the X button. I guess this is expected?
  1. Right, this is expected.
  2. Right, we just inform the user to switch back to the browser.

Copy link

sonarcloud bot commented Jul 10, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

Copy link
Contributor

@giannis-rdx giannis-rdx left a comment

Choose a reason for hiding this comment

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

great job! 💯

@sergiupuhalschi-rdx sergiupuhalschi-rdx merged commit 3937fbf into main Jul 11, 2024
8 of 9 checks passed
@sergiupuhalschi-rdx sergiupuhalschi-rdx deleted the feature/ABW-3435-home-cards-carousel branch July 11, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants