Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Fix: #4273 Built Send Crypto Screen #4380

Merged
merged 6 commits into from Oct 28, 2021
Merged

Conversation

nuo-xu
Copy link
Contributor

@nuo-xu nuo-xu commented Oct 21, 2021

Summary of Changes

  • Built Send Crypto Screen UI
  • Integrated the screen with token fetching/selection/balance

This pull request fixes #4273

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Screenshots:

simulator_screenshot_C15EAA98-BA84-4483-8060-A40AC488C9AE

simulator_screenshot_9BC6C333-EE90-460A-8CC8-FE4062E83249

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

BraveWallet/Crypto/BuySwapSwap/SendTokenView.swift Outdated Show resolved Hide resolved
Comment on lines 83 to 86
Image("brave.clipboard")
.resizable()
.aspectRatio(contentMode: .fit)
.frame(width: length, height: length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Set a font on this instead, brave.clipboard is a custom SF Symbol so no need to make it resizable/aspect-fit/frame modifiers

BraveWallet/Crypto/BuySwapSwap/SendTokenView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/BuySwapSwap/SendTokenView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/BuySwapSwap/SendTokenView.swift Outdated Show resolved Hide resolved
}

public func accountsChanged() {
fetchAssetBalance()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't think you have to re-fetch asset balance when an account is added/removed as long as the selected account still exists. Maybe add a check here to see if selected account exists in in the keyring.accountInfos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think you have to re-fetch asset balance when an account is added/removed as long as the selected account still exists. Maybe add a check here to see if selected account exists in in the keyring.accountInfos

I wasn't too sure what this protocol is for that's why I fetch the assets there. But after your explanation, i think fetch assets in selectedAccountChanged is sufficient.

BraveWallet/WalletStrings.swift Outdated Show resolved Hide resolved
BraveWallet/WalletStrings.swift Outdated Show resolved Hide resolved
BraveWallet/WalletStrings.swift Outdated Show resolved Hide resolved
BraveWallet/WalletStrings.swift Outdated Show resolved Hide resolved
Comment on lines 95 to 100
NSLayoutConstraint.activate([
imageView.centerYAnchor.constraint(equalTo: view.centerYAnchor),
imageView.centerXAnchor.constraint(equalTo: view.centerXAnchor),
imageView.widthAnchor.constraint(equalToConstant: 200),
imageView.heightAnchor.constraint(equalToConstant: 200),
])
Copy link
Collaborator

Choose a reason for hiding this comment

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

use SnapKit instead

Comment on lines 67 to 71
private lazy var captureSession: AVCaptureSession = {
let session = AVCaptureSession()
session.sessionPreset = AVCaptureSession.Preset.high
return session
}()
Copy link
Collaborator

Choose a reason for hiding this comment

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

private let captureSession = AVCaptureSession().then {
  $0.sessionPreset = .high
}

BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
let imageView = UIImageView().then {
$0.image = UIImage(named: "camera-overlay")
$0.contentMode = .center
$0.translatesAutoresizingMaskIntoConstraints = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

isAccessibilityElement = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isAccessibilityElement = false

it's right below at 159 😀

import AVFoundation
import Shared

struct WalletScannerView: UIViewControllerRepresentable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As per conversation: do something like this

struct WalletScannerView: View {
  @Binding var address: String
  @State private var isErrorPresented: Bool = false
  @Environment(\.presentationMode) @Binding private var presentationMode
  
  var simulatorBody: some View {
    
  }
  
  var body: some View {
    NavigationView {
    #if targetEnvironment(simulator)
    Text("Unable to scan on simulator")
      .onAppear {
        DispatchQueue.main.asyncAfter(deadline: .now() + 1) {
          address = ""
          presentationMode.dismiss()
        }
      }
    #else
    _WalletScannerView(address: _address)
      .ignoresSafeArea()
      .alert()
    #endif
    }
    .toolbar {
      ToolbarItemGroup(placement: .cancellationAction) {
        Text("")
      }
    }
  }
}

BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
BraveWallet/Crypto/Scanner/WalletScannerView.swift Outdated Show resolved Hide resolved
Comment on lines 225 to 249
guard let readableObject = metadataObject as? AVMetadataMachineReadableCodeObject else {
isFinishScanning = true
isErrorPresented = true
return
}
guard let stringValue = readableObject.stringValue else {
isFinishScanning = true
isErrorPresented = true
return
}
guard isFinishScanning == false else { return }

found(code: stringValue)
guard stringValue.isAddress else {
isFinishScanning = true
isErrorPresented = true
return
}

toAddress = stringValue
isFinishScanning = true
dismiss()
} else {
isFinishScanning = true
isErrorPresented = true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can be all combined into a simpler flow

if !isFinishScanning, 
    let stringValue = (metadataObjects.first as? AVMetadataMachineReadableCodeObject)?.stringValue, 
    stringValue.isAddress {
  address = stringValue
  dismiss()
} else {
  isErrorPresented = true
}
isFinishScanning = true

Are we treating non-address as errors as well? Are we going to have a special error message for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked other existed scanners they don't have output validation. We can treat it as invalid data to have a popup so that user knows the scan did happen but the data is wrong.

BraveWallet/Crypto/Stores/Address.swift Outdated Show resolved Hide resolved
Also removes recents commented out stuff
@nuo-xu nuo-xu merged commit 556ee47 into brave-wallet Oct 28, 2021
@nuo-xu nuo-xu deleted the feature/wallet-sent-crypto branch October 28, 2021 16:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants