🦺[Tech debt] Add configuration to hide / unhide card tab #47

kcw-grunt · 2021-05-11T22:54:28Z

Problem

The users are using Litewallet and some are located outside the US which means they are unable to use the Litecoin Card. This causes major support issues and generally makes Litewallet bad as a product. There is also alot confusion about what the Card does and who does what.

Fixes device locale and the users ability to see the Card tab and it will reduce the confusion

Approach

Rather than refactor the UITabBarViewController into a TabView a US and Non-US TabViewController is used
Added checks at launch to verify users locale
Added tests to make sure that if the user is using a non-English language but still in the US, they are shown the Card tab
Added tests that when the user is in the US, the correct views are shown , or at least the boolean switch is working.

Keywords

Closes #40

- Added test at launch of locale

Implemented switching of the tabs

- Added a EXUS ViewContoller - Added the setup code for both EXUS and TabBar ViewContollers - Basic tests

Added the Locale Detail view

kcw-grunt · 2021-05-11T22:57:48Z

cc: @JasperHeist

JCHeist

@kcw-grunt , I know you didn't add me as a reviewer but I took a look at it anyways and don't have any issues. I see we have a test that failed. Im not too familiar with how checks work on github, but I guess we should look at that.
I still can't build but i don't think you were mean this pull request would fix that when you mentioned it was a start. So, I can't test it locally but let me know if you'd like to me to approve it, because I didn't find any issues.

kcw-grunt · 2021-05-12T15:40:59Z

@kcw-grunt , I know you didn't add me as a reviewer but I took a look at it anyways and don't have any issues. I see we have a test that failed. Im not too familiar with how checks work on github, but I guess we should look at that.
I still can't build but i don't think you were mean this pull request would fix that when you mentioned it was a start. So, I can't test it locally but let me know if you'd like to me to approve it, because I didn't find any issues.

Thanks @JasperHeist really appreciate it. I cc'ed you while we get you into the repo org.
Later I will set you up accordingly.

- Clean up of code - Added SwiftUI Locale Detail View

JCHeist

If this sounds good, it seems that all we would need to do is have different list of Storyboard names and ID's. The other changes to the file, the only other change i found is the switch statements which would be fine to stay in the BaseTabViewController.

JCHeist · 2021-05-13T11:08:10Z

loafwallet/NonUSTabBarViewController.swift

+
+import UIKit
+import Foundation 
+


Is it feasible to create a BaseTabBarViewController class from this and inherit from that to create the US TabBarViewController since the behavior is nearly identical?

Actually, I would prefer to refactor to a TabView and set a conditional layout . The problem is this code is quite old and we have limited time to put in the changes.
The app leverages a weird view stack, which is not typical but was inherited from the Bread fork.

Let me review the switch statements...👀

@kcw-grunt yeah agreed with @JasperHeist to have a BaseTabBarViewController since the both TabBarViewController and NonUSTabBarViewController are almost the same. But as you said it could take time . So for now let's add it into the technical debt issues

- meeting switch block indentation standards

JCHeist

Ok sounds good to me 👍

kcw-grunt · 2021-05-13T17:56:33Z

Ok sounds good to me 👍

Thanks for the 🍀.

As you have probably already figured out , the code base is in need of a serious refactor. Yet, we need to make progress as well.

mosadialiou · 2021-05-13T18:02:19Z

loafwallet/AppDelegate.swift

+
+        let suffix = String(localeId.suffix(3))
+
+        if suffix == "_US" {


@kcw-grunt we can simplify the expression like this:
UserDefaults.userIsInUSA = suffix == "_US"

Hey @mosadialiou this is to allow people that are in the US but speak other languages . So, someone that speaks French and has their phone set for french but are in say, New York . This region code would be "fr_US".
So this code truncates the fr.

@kcw-grunt yeah I understand. I am talking about the code. You can simplify it like I said!

loafwallet/LocaleChangeViewModel.swift

loafwallet/src/ModalPresenter.swift

mosadialiou · 2021-05-13T18:45:47Z

loafwallet/MainViewController.swift

@@ -91,40 +91,61 @@ class MainViewController : UIViewController, Subscriber, LoginViewControllerDele
    }

    func didUnlockLogin() {


@kcw-grunt refactor this method like this:

func didUnlockLogin() { var vc : UIViewController if UserDefaults.userIsInUSA { vc = UIStoryboard.init(name: "Main", bundle: nil) .instantiateViewController(withIdentifier: "TabBarViewController") as? TabBarViewController } else { vc = UIStoryboard.init(name: "Main", bundle: nil) .instantiateViewController(withIdentifier: "NonUSTabBarViewController") as? NonUSTabBarViewController } vc.store = self.store vc.walletManager = walletManager addChildViewController(vc, layout: { vc.view.constrain(toSuperviewEdges: nil) vc.view.alpha = 0 vc.view.layoutIfNeeded() }) UIView.animate(withDuration: 0.3, delay: 0.1, options: .transitionCrossDissolve, animations: { vc.view.alpha = 1 }) { (finished) in NSLog("\(UserDefaults.userIsInUSA ? US : Ex US) MainView Controller presented") } }

Oh that won't work. The classes are custom and inherit so the values can't be passed to the NonUS and Tab ViewControllers

@kcw-grunt hmm I see. Can you do this 👇 ?

func didUnlockLogin() { var vc : UIViewController if UserDefaults.userIsInUSA { guard let usaVC = UIStoryboard.init(name: "Main", bundle: nil) .instantiateViewController(withIdentifier: "TabBarViewController") as? TabBarViewController else { NSLog("TabBarViewController not intialized") return } usaVC.store = self.store usaVC.walletManager = walletManager vc = usaVC } else { guard let exUSAVC = UIStoryboard.init(name: "Main", bundle: nil) .instantiateViewController(withIdentifier: "NonUSTabBarViewController") as? NonUSTabBarViewController else { NSLog("NonUSTabBarViewController not intialized") return } exUSAVC.store = self.store exUSAVC.walletManager = walletManager vc = exUSAVC } addChildViewController(vc, layout: { vc.view.constrain(toSuperviewEdges: nil) vc.view.alpha = 0 vc.view.layoutIfNeeded() }) UIView.animate(withDuration: 0.3, delay: 0.1, options: .transitionCrossDissolve, animations: { vc.view.alpha = 1 }) { (finished) in NSLog("\(UserDefaults.userIsInUSA ? US : Ex US) MainView Controller presented") } }

mosadialiou · 2021-05-13T18:57:36Z

loafwallet/NonUSTabBarViewController.swift

+
+import UIKit
+import Foundation 
+


@kcw-grunt yeah agreed with @JasperHeist to have a BaseTabBarViewController since the both TabBarViewController and NonUSTabBarViewController are almost the same. But as you said it could take time . So for now let's add it into the technical debt issues

kcw-grunt · 2021-05-13T19:19:02Z

@mosadialiou going to merge this after the changes you recommended were brought up and tests pass. TYVM.

mosadialiou · 2021-05-13T19:44:06Z

@kcw-grunt can you please attach here a screenshot or clip of the localchangeView

mosadialiou

LGTM

JCHeist · 2021-05-14T03:34:50Z

Ok sounds good to me 👍

Thanks for the 🍀.

As you have probably already figured out , the code base is in need of a serious refactor. Yet, we need to make progress as well.

A code base in serious need of a refactor? I've never HEARD of such a thing 🤣 /s

kcw-grunt added 5 commits May 11, 2021 23:46

- Added tests for the Card test group

15855ee

- Added test at launch of locale

Added NonUSTabBar

c12f7ae

Implemented switching of the tabs

Checked that basic switching works

83faefc

- Added a EXUS ViewContoller - Added the setup code for both EXUS and TabBar ViewContollers - Basic tests

Added cell for locale

d51e50e

Added the Locale Detail view

Added “Current Locale” localizations

3d94855

JCHeist reviewed May 12, 2021

View reviewed changes

kcw-grunt requested a review from mosadialiou May 12, 2021 15:41

Polished tests, added locale detail view

c4e8f03

- Clean up of code - Added SwiftUI Locale Detail View

kcw-grunt requested a review from JCHeist May 13, 2021 09:37

JCHeist suggested changes May 13, 2021

View reviewed changes

pre review comments

4c68e78

- meeting switch block indentation standards

kcw-grunt requested a review from JCHeist May 13, 2021 15:45

JCHeist approved these changes May 13, 2021

View reviewed changes

mosadialiou suggested changes May 13, 2021

View reviewed changes

per review comments.

81420bd

kcw-grunt requested a review from mosadialiou May 13, 2021 19:15

mosadialiou approved these changes May 13, 2021

View reviewed changes

removed flaky test

2f89a91

kcw-grunt force-pushed the techdebt/hide-unhide-card-40 branch from 3234250 to 2f89a91 Compare May 13, 2021 23:16

kcw-grunt merged commit 0b5ba31 into develop May 13, 2021

kcw-grunt deleted the techdebt/hide-unhide-card-40 branch May 15, 2021 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🦺[Tech debt] Add configuration to hide / unhide card tab #47

🦺[Tech debt] Add configuration to hide / unhide card tab #47

kcw-grunt commented May 11, 2021 •

edited

Loading

kcw-grunt commented May 11, 2021

JCHeist left a comment

kcw-grunt commented May 12, 2021

JCHeist left a comment

JCHeist May 13, 2021

kcw-grunt May 13, 2021

kcw-grunt May 13, 2021

mosadialiou May 13, 2021

JCHeist left a comment

kcw-grunt commented May 13, 2021

mosadialiou May 13, 2021

kcw-grunt May 13, 2021

mosadialiou May 13, 2021

mosadialiou May 13, 2021

kcw-grunt May 13, 2021

mosadialiou May 13, 2021 •

edited

Loading

mosadialiou May 13, 2021

kcw-grunt commented May 13, 2021

mosadialiou commented May 13, 2021

mosadialiou left a comment

JCHeist commented May 14, 2021

		@@ -91,40 +91,61 @@ class MainViewController : UIViewController, Subscriber, LoginViewControllerDele
		}

		func didUnlockLogin() {


		import UIKit
		import Foundation


		import UIKit
		import Foundation

🦺[Tech debt] Add configuration to hide / unhide card tab #47

🦺[Tech debt] Add configuration to hide / unhide card tab #47

Conversation

kcw-grunt commented May 11, 2021 • edited Loading

Problem

Approach

Keywords

kcw-grunt commented May 11, 2021

JCHeist left a comment

Choose a reason for hiding this comment

kcw-grunt commented May 12, 2021

JCHeist left a 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

JCHeist left a comment

Choose a reason for hiding this comment

kcw-grunt commented May 13, 2021

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

mosadialiou May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcw-grunt commented May 13, 2021

mosadialiou commented May 13, 2021

mosadialiou left a comment

Choose a reason for hiding this comment

JCHeist commented May 14, 2021

kcw-grunt commented May 11, 2021 •

edited

Loading

mosadialiou May 13, 2021 •

edited

Loading