[ABW-3439] Card Carousel #1187

kugel3 · 2024-06-28T05:44:19Z

Jira ticket: ABW-3439
Jira ticket: ABW-3443

Description

A carousel showing various action cards at the top of the home screen.

CyonAlexRDX · 2024-06-28T12:40:35Z

RadixWallet/Clients/CardCarouselClient/CardCarouselClient+Interface.swift

+
+// MARK: - CarouselCard
+public enum CarouselCard: Hashable, Sendable {
+	case threeSixtyDegrees


Do we plan on letting the Cards be an enum? Is it because we need to know what to do when the card is pressed, and we dont wanna represent that with a closure? Especially not if this is to be moved into Sargon?

It looks like the repertoire of cards is very limited, and they are completely static, so it seems like a good model. The action will be known at compile time, not sure it makes sense to store it on the model, it can just be handled in the reducer, or possibly the client.

RadixWallet/Clients/CardCarouselClient/CardCarouselClient+Interface.swift

...s/Resources/Assets.xcassets/CardCarousel/carousel_icon_continueOnDapp.imageset/Contents.json

...s/Resources/Assets.xcassets/CardCarousel/carousel_full_background_360.imageset/Contents.json

RadixWallet/Features/HomeFeature/Coordinator/Home+View.swift

RadixWallet/Features/CardCarousel/CardCarousel+View.swift

RadixWallet/Clients/CardCarouselClient/CardCarouselClient+Live.swift

GhenadieVP · 2024-06-28T09:13:44Z

RadixWallet/Features/CardCarousel/CardCarousel+Reducer.swift

+
+	@CasePathable
+	public enum ViewAction: Equatable, Sendable {
+		case didAppear


usually this is a task

GhenadieVP · 2024-07-05T12:55:40Z

RadixWallet/Clients/AppEventsClient/AppEventsClient+Live.swift

+
+		return .init(
+			handleEvent: { event in
+				switch event {


Hmm, not 100% sure about this approach. Should AppEventsClient perform some actions on the events, or should it just propagate the events?
In the later case the, HomeCardsClients would listen for the events emitted by the AppEventsClient and perform the actions on its own.

wdyt?

I liked this idea a lot, so gave it a try here. However, there seems to be a race condition error since the .appStarted event is never received by the listener. I even added this handy method to be called by the AppEventsClient on its initialization, to make sure the HomeCardsClient subscribes to events before the first one is dispatched, but problem was still present.

We can work on some delays but even having the AppsEventsClient starting the HomeCardsClient (and whatever other listener client we would have in the future) goes against the whole reasoning behind this change.

resolved with AsyncReplaySubject

GhenadieVP · 2024-07-05T12:57:02Z

RadixWallet/Clients/AppsFlyerClient/AppsFlyerClient+Live.swift

+				if deepLink.isDeferred, let value = deepLink.clickEvent["deep_link_value"] as? String {
+					appEventsClient.handleEvent(.deepLinkReceived(value))


We already have DeepLinkHandlerClient, could we extend it to handle the deferred deepLink?

mm I am not sure I prefer it this way, since DeepLinkHandlerClient is about non deferred deep links. The logic for deferred vs non deferred is totally different, so not sure if it wouldn't add more confusion to put it there just for the sake of being a deep link.

GhenadieVP · 2024-07-05T12:58:45Z

RadixWallet/Clients/HomeCardsClient/HomeCardsClient+Interface.swift

+	public var walletStarted: WalletStarted
+	public var walletCreated: WalletCreated
+	public var deepLinkReceived: DeepLinkReceived


these should express some command, like handle..., or on.... similar to the remove above.

ignored as per changes to AppEventClient

GhenadieVP · 2024-07-05T13:43:07Z

RadixWallet/Features/CardCarousel/CardCarousel+View.swift

+		@MainActor
+		private var coreView: some SwiftUI.View {
+			TabView(selection: $selectedCardIndex) {
+				ForEach(Array(store.cards.enumerated()), id: \.1) { index, card in


could we use \.element instead of \.1?

What is the purpose of enumerating it and then use element as id, instead of just doing \id:.self? And stepping back, what was wrong with having the card be identifiable?

We use the index to tag each card, which is necessary for the positionIndicator defined below. We can extend the card model (now defined in Sargon), and make it Identifiable by defining an id for each. Same result, just need some extra code.

The problem with that is that swiftui will not be able to handle insertion and removal animations properly if the the index is the id. For example if you have four cards and remove the third card, with id and index 2, the collection will still have a card with id 2, but it will be the one that used to have id 3. In short, using index as id with a dynamic collection is not supported in SwiftUI. That is also why I called my helper type that wraps up the functionality you use here, "ForEachStatic".

We aren't using the index as the id, we are using the \.element to identify them.

We are using the index to tag them, and that works correctly on your example since after removing card with index: 2, the index (and tag) is properly updated with the new set of elements.

Ah, alright, so you use element for id, and index only for the tag, for the benefit of the tabview.

I would still consider using the same tag and id, and map it in the reducer, since in there you have access to both the elements and their offsets.

GhenadieVP · 2024-07-05T13:47:30Z

RadixWallet/Features/CardCarousel/CardCarousel+View.swift

+
+		private var positionIndicator: some SwiftUI.View {
+			HStack(spacing: spacing) {
+				if store.cards.count > 1 {


Minor: maybe it should be outside of positionIndicator, just to not add an empty HStack I guess

GhenadieVP · 2024-07-05T13:50:30Z

RadixWallet/Features/CardCarousel/CardCarousel+View.swift

+				.padding([.top, .leading], .medium2)
+				.padding(.trailing, trailingPadding)
+				.padding(.bottom, .small1)
+				.frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topLeading)


This is like a lot of padding + frame, is it really needed?

This makes it so that the frame grows as much as it can, instead of hugging the content. It's very similar to using say an HSTack with Spacer's.

GhenadieVP · 2024-07-05T13:50:55Z

RadixWallet/Features/CardCarousel/CardCarousel+View.swift

+			Button(action: closeAction) {
+				Image(asset: AssetResource.close)
+					.resizable()
+					.frame(width: .medium3, height: .medium3)
+					.tint(.app.gray1)
+					.padding(.small2)
+			}


We have already close button, surprised that this is needed

The close button is intended to be used in a toolbar so it has a bigger padding, which makes the X overlap with the background/dapp image. That's why I changed it to this manual implementation.

CloseButton does not seem to have any padding, but anyway would be nice to have a single definition of CloseButton if possible.

CloseButton defines a size of 44x44, meaning the asset (24x24) will have a 10 padding on each side.

In this case, we want the asset to be of 16x16 and to have a padding of 8 on top & trailing sides.

I can extend the CloseButton to adapt for each type.

GhenadieVP · 2024-07-05T13:51:45Z

RadixWallet/Features/CardCarousel/CardCarousel+View.swift

+		}
+	}
+
+	private var trailingPadding: CGFloat {


Not quite getting what this is for, I guess I need to play with it.

basically how much space we must leave empty from the trailing side of the view. Depending on its content, we have more or less space that we can use without the text overlapping with an image.

Original implementation had a fixed value, but that would leave very little space for some views on smaller devices (as iPhone SE) and content would look trimmed.

The reason it was fixed is that the image we want to avoid covering is fixed size, because the view has a fixed height. That in turn is because tabview can't handle flexible heights very well.

kugel3 · 2024-07-07T20:02:39Z

I saw a question about the dummy views, though I can't find it anymore. Their purpose is to show the teased cards on the sides. The tabview itself does not reach the end of the screen, it is only as wide as the card, plus half the inter card spacing on each side.

GhenadieVP · 2024-07-10T06:28:36Z

RadixWallet/Features/CreateAccount/Coordinator/CreateAccountCoordinator+Reducer.swift

+			if state.config.shouldSendWalletCreatedEvent {
+				appEventsClient.handleEvent(.walletCreated)
+			}


This event should be added when the onboarding is completed in App+Reducer, as by definition the onboarding happens when user creates a new Wallet.

But in such case, it could be because the user restored a wallet

Correct, then it would be better to have this code in OnboardingCoordinator when the CreateAccount completes.

CyonAlexRDX reviewed Jun 28, 2024

View reviewed changes

kugel3 force-pushed the ABW-3439_Card-carousel branch from 4665695 to b2d6ebb Compare June 29, 2024 11:35

kugel3 added 17 commits June 30, 2024 21:33

wip

81bf003

wip

c1ddf49

wip

39a880b

wip

ca0373a

wip

5ce4ed4

wip

da8f3d7

wip

af2b0f2

works

6cb0b7e

Simpler

dc784df

refactor

4939e65

TEMP add development asset

0072003

Better card

dc98d28

Refactor

489c66b

CardCarouselClient

b62daca

Flexible size

1060d2c

wip

5fac7ed

wip

2a6e024

kugel3 force-pushed the ABW-3439_Card-carousel branch from 52334ba to 2a6e024 Compare June 30, 2024 19:42

kugel3 added 2 commits June 30, 2024 22:20

refactored

24b0205

Small refactor

84dc42e

matiasbzurovski reviewed Jul 2, 2024

View reviewed changes

RadixWallet/Features/CardCarousel/CardCarousel+View.swift Outdated Show resolved Hide resolved

matiasbzurovski and others added 2 commits July 5, 2024 11:41

Merge branch 'main' into ABW-3439_Card-carousel

08ddac9

Home Cards logic and UI updates (#1199)

1ac02ee

GhenadieVP reviewed Jul 5, 2024

View reviewed changes

matiasbzurovski added 2 commits July 5, 2024 17:07

Updates from Demo

0b0479e

first round of feedback

a79a0f5

matiasbzurovski added 3 commits July 8, 2024 12:17

Merge branch 'main' into ABW-3439_Card-carousel

179daae

Extend CloseButton to support a kind

3f4f83d

Refactor AppEventsClient

800a610

matiasbzurovski force-pushed the ABW-3439_Card-carousel branch from 6c36818 to 800a610 Compare July 8, 2024 11:55

matiasbzurovski added 5 commits July 8, 2024 16:18

Add bootstrap client

b0f8ac4

Add bootstrap client to project

345d591

Use new Sargon methods

4b927fc

Send wallet created event only when it corresponds

7c05410

Bump Sargon and manually set user defaults

0bf524f

GhenadieVP reviewed Jul 10, 2024

View reviewed changes

matiasbzurovski and others added 5 commits July 10, 2024 10:24

Remove Home radix banner

50da244

Remove Connector banner from Settings

6f9f6f0

Merge branch 'main' into ABW-3439_Card-carousel

1be9f16

Move to event to OnboardingCoordinator

3d3c477

Split delegate calls in 2

0cab947

GhenadieVP approved these changes Jul 11, 2024

View reviewed changes

GhenadieVP merged commit 04c658e into main Jul 11, 2024
6 checks passed

GhenadieVP deleted the ABW-3439_Card-carousel branch July 11, 2024 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ABW-3439] Card Carousel #1187

[ABW-3439] Card Carousel #1187

kugel3 commented Jun 28, 2024 •

edited by jira bot

Loading

CyonAlexRDX Jun 28, 2024

kugel3 Jun 28, 2024

GhenadieVP Jun 28, 2024

GhenadieVP Jul 5, 2024

matiasbzurovski Jul 8, 2024

matiasbzurovski Jul 8, 2024

GhenadieVP Jul 5, 2024

matiasbzurovski Jul 8, 2024

GhenadieVP Jul 5, 2024

matiasbzurovski Jul 8, 2024

GhenadieVP Jul 5, 2024

kugel3 Jul 7, 2024

matiasbzurovski Jul 8, 2024

kugel3 Jul 8, 2024

matiasbzurovski Jul 9, 2024

kugel3 Jul 9, 2024

GhenadieVP Jul 5, 2024

GhenadieVP Jul 5, 2024

matiasbzurovski Jul 5, 2024

kugel3 Jul 7, 2024

GhenadieVP Jul 5, 2024

matiasbzurovski Jul 5, 2024

GhenadieVP Jul 8, 2024

matiasbzurovski Jul 8, 2024 •

edited

Loading

GhenadieVP Jul 5, 2024

matiasbzurovski Jul 5, 2024

kugel3 Jul 7, 2024

kugel3 commented Jul 7, 2024

GhenadieVP Jul 10, 2024

matiasbzurovski Jul 10, 2024

GhenadieVP Jul 10, 2024

		if deepLink.isDeferred, let value = deepLink.clickEvent["deep_link_value"] as? String {
		appEventsClient.handleEvent(.deepLinkReceived(value))

[ABW-3439] Card Carousel #1187

[ABW-3439] Card Carousel #1187

Conversation

kugel3 commented Jun 28, 2024 • edited by jira bot Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiasbzurovski Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kugel3 commented Jul 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kugel3 commented Jun 28, 2024 •

edited by jira bot

Loading

matiasbzurovski Jul 8, 2024 •

edited

Loading