-
Notifications
You must be signed in to change notification settings - Fork 180
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
[W3M] UI + Sign integration #843
Conversation
@@ -0,0 +1,14 @@ | |||
import SwiftUI | |||
|
|||
struct ActivityIndicator: UIViewRepresentable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it iOS 13 😅?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ProgressView is iOS 14 :|
} | ||
|
||
private func closeButton() -> some View { | ||
Button(action: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Button {
// action
} label: {
// view
}
Not critical but a bit cleaner, if you like
viewModel.navigateTo(.qr) | ||
} | ||
}, label: { | ||
Image("qr_large", bundle: .module) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it's not final etc, but would be nice to have all the assets to be stored in i.e. enum in the future.
enum Icons {
static let qrLarge = "qrLarge"
}
...
Image(Icons.qrLarge)
|
||
private func copyButton() -> some View { | ||
Button(action: { | ||
UIPasteboard.general.string = viewModel.uri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to call this on ViewModel
so we can test it
.frame(height: 60) | ||
.overlay( | ||
Text(viewModel.destination.contentTitle) | ||
.font(.system(size: 20).weight(.semibold)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same: some data structure for the fonts would be more convenient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do once there is some clear typography defined in figma which is not the case afaik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sexy 😍
Commented-out code depends on Networking changes which will be in the next PR