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-3213] Address Details #1100

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

[ABW-3213] Address Details #1100

wants to merge 40 commits into from

Conversation

matiasbzurovski
Copy link
Contributor

@matiasbzurovski matiasbzurovski commented Apr 26, 2024

Jira ticket: ABW-3213
Jira ticket: ABW-3256

Description

This PR adds a sheet presented every time the user taps on an AddressView with a LedgerIdentifiable.Address associated.

Video

demo.mov

@matiasbzurovski matiasbzurovski added the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Apr 26, 2024
@@ -31,6 +31,21 @@ extension Gradient {
]
}

extension LinearGradient.App {
public var brand1: LinearGradient {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up not being used on these designs but decided to keep it as we will probably need it somewhere else (it is part of the design system.

@GhenadieVP
Copy link
Contributor

GhenadieVP commented May 3, 2024

@matiasbzurovski Note: This is to be merged for the next 1.6.0 release if Android manages to implement it, otherwise it will go in 1.7.0 release.

@matiasbzurovski matiasbzurovski removed the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Jun 25, 2024
}

private extension AddressDetails.ViewState {
static func colorised(address: LedgerIdentifiable.Address) -> AttributedString {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be either a local private function, or, if you might want to use it somewhere else, a var on LedgerIdentifiable.Address or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to computate it only once

Copy link
Contributor

Choose a reason for hiding this comment

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

The ViewState is actually re-computed every time anything changes, as is the entire SwiftUI View, so I don't think it makes any difference.

But either way, my idea was just to change the ergonomics to something like this:

let addressString = address.colorizedString

It might be that Observable is smart enough to only recompute it when the underlying address changes if you do it like that. When it's in the ViewState, any change to any property on the ViewState would recompute everything.

Seems like it could be useful more generally, btw.

// MARK: - Helpers

private extension OnLedgerEntity.Resource {
var resourceTitle: String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have something with a very similar name, which returned metadata.symbol ?? metadata.name, but I can't find it right now. If it still exists somewhere it might be a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably this title defined on OnLedgerEntity.Metadata.

The one I defined is private anyway, and is an improved version of it. smartResourceTitle?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't say it's improved, it's different, and only makes sense here

Copy link
Contributor

Choose a reason for hiding this comment

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

So I would just make it a private computed property in the View

Copy link
Contributor

@kugel3 kugel3 left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor suggestions

.foregroundColor(.app.gray1)
Text(address)
private func qrCode(qrImage: Loadable<CGImage>) -> some SwiftUI.View {
ZStack {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the ZStack do here? It looks like each branch only returns a single element, so it would seem that it would be enough to put @ViewBuilder on the function

// MARK: - Enlarged view
private extension AddressDetails.View {
var enlargedView: some SwiftUI.View {
Group {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of the Group?

private var enlargedViewBackrgound: some View {
Color.black.opacity(0.2)
.frame(width: UIScreen.main.bounds.width * 2, height: UIScreen.main.bounds.height * 2)
.contentShape(Rectangle())
Copy link
Contributor

Choose a reason for hiding this comment

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

contentShape controls the tappable area, when you have a part of the view that has alpha=0, but all of this view is black, so isn't it already tappable?

private func loadTitle(address: LedgerIdentifiable.Address) async throws -> String? {
switch address {
case let .account(address, _):
let account = try await accountsClient.getAccountByAddress(address)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it's a matter of taste but it looks like all these branches could be written as one-liners, without a return.

try await accountsClient.getAccountByAddress(address).displayName.rawValue

Comment on lines +112 to +115
await send(.internal(.loadedTitle(TaskResult {
try await loadTitle(address: address)
})))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Much cleaner!

@@ -1,18 +1,21 @@
// MARK: - AddressDetails
public struct AddressDetails: Sendable, FeatureReducer {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add @Reducer to this. Then you need to also put public typealias Action = FeatureAction<Self> explicitly, otherwise the compiler gets confused.

@kugel3
Copy link
Contributor

kugel3 commented Jun 28, 2024

Now the dismiss doesn't actually work for me in your PR. But putting@Environment(\.dismiss) var dismiss in the View instead does work.

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

3 participants