[ABW-3723] Resource hiding/unhiding

[matiasbzurovski]

Jira ticket: ABW-3723

Description

This PR allows to hide/unhide resources (fungible, non-fungible & pool units)

How to test

Play with your user as the demo video shows.

Video

Video

PR submission checklist

• I have tested account to account transfer flow and have confirmed that it works

[CyonAlexRDX]

personal preferences, I dislike extension [Element] syntax - since it makes it impossible to search for all Array extensions and all Dictionary extensions individually. But I think I'm the only one to dislike it and we are using it elsewhere.

[matiasbzurovski]

I actually used to add extensions like this as extension Array where Element == XYZ before, would you prefer it that way?

[CyonAlexRDX]

Yes I prefer that, but then we have fragmentation in the code base. I think this is the most common syntax we use, so maybe best stick to it

[CyonAlexRDX]

Excellent!

[CyonAlexRDX]

annoying that ResultBuilders does not support guard or otherwise return early :/
SE-0289 states

guard is provisionally ill-formed when it appears within a transformed function. Situations in which this statement may appear may be supported in the future, such as when the statement does not produce a partial result.

[matiasbzurovski]

right, or ternary operators so that we could do something like this on the body:

store.fungibles.isEmpty ? empty : fungibles

and remove the conditional check inside var fungibles

[kugel3]

You could put the if-else at the parent level, and let the fungibles var only be called when not empty. That would be pretty clean. I think it should be called emptyState though, not just empty. And I would make it a nested view, so you would use it with EmptyState() instead, it doesn't seem to use any variables.

[matiasbzurovski]

well this is debatable. I prefer looking at the body and getting a clear and quick vision of what's going on. In my opinion it looks cleaner to see this:

LazyVStack(alignment: .leading, spacing: .large3) {
	Text(L10n.HiddenAssets.text)
		.textStyle(.body1HighImportance)
		.foregroundColor(.app.gray2)

	header(L10n.HiddenAssets.fungibles)
	fungibles

	header(L10n.HiddenAssets.nonFungibles)
	nonFungibles

	header(L10n.HiddenAssets.poolUnits)
	poolUnits
}

than this:

LazyVStack(alignment: .leading, spacing: .large3) {
	Text(L10n.HiddenAssets.text)
		.textStyle(.body1HighImportance)
		.foregroundColor(.app.gray2)

	header(L10n.HiddenAssets.fungibles)
	if store.fungibles.isEmpty {
		empty
	} else {
		fungibles
	}

	header(L10n.HiddenAssets.nonFungibles)
	if store.nonFungibles.isEmpty {
		empty
	} else {
		nonFungibles
	}

	header(L10n.HiddenAssets.poolUnits)
	if store.poolUnits.isEmpty {
		empty
	} else {
		poolUnits
	}
}

[kugel3]

Sure, but the second option makes the variables cleaner. But my main point is that it is better to not hide the empty state, since that is in fact a top-level thing, I don't think it should be hidden.

[kugel3]

It looks like you scope the destinationStore and then use the HideAsset store instead? What is going on here?

[matiasbzurovski]

I scope the destinationStore to tell when this ConfirmationView alert should be presented. Given the alert doesn't have a reducer (it is a plain SwiftUI view that have a closure for each button that can be tapped: confirm and cancel), we just need to handle the button tap on this reducer.

[kugel3]

Did you try using the new style, where you just define something like:

extension HideAsset {
  @Reducer(state: .hashable, action: .equatable)
  enum Destination {
    case confirmation(ConfirmationAction)
  }
}

[matiasbzurovski]

how do you make the parent conform to FeatureReducer ?

[kugel3]

You can do put @Reducer(state: .hashable, action: .equatable) on the Destination if that is what you have in mind? But it was a genuine question, it seems that it only will play nicely with FeatureReducer sometimes.

[kugel3]

If the macro is causing issues when combined with FeatureReducer, feel free to skip the latter, and implement the needed actions etc manually.

[kugel3]

I suppose this should also be public?

[matiasbzurovski]

nothing needs to be public actually :)

[kugel3]

Sure, but since the struct and all the other properties are public, this one should be as well. There is no reason for them to have different access levels, it only causes confusion.

[matiasbzurovski]

IMO the confusion is caused by leaving the others public, and what we should do is actually remove the access modifier from those.

[kugel3]

Previously we had separate modules for every feature, so everything had to be public. Now there is no technical reason for it, but we should at the very least be consistent within a feature.

[kugel3]

Same here, since the empty state is not specific to accounts, I don't think it needs to be part of var accounts

[GhenadieVP]

I suppose you added this for the scenario when viewing the details of an asset from TX Review, but should we allows users to hide an asset from TX Review in the first place?

[matiasbzurovski]

according to my demo on Friday, yes