[WEB-700] FB Auth Deprecation | SetYourPassword UI

[scottkicks]

📲 What

UI for a new SetYourPassword screen.

🤔 Why

As we start to deprecate signing in with facebook, we need to make sure that specific users create a password.
These users have connected via facebook but never needed to create one and so before we remove facebook auth altogether we need to make sure they can still access their accounts.

🛠 How

I created a new SetYourPasswordViewController & SetYourPasswordViewModel based on these designs

👀 See

✅ Acceptance criteria

• This screen isn't hooked up yet, I just wanted some feedback on building the UI first. I've been adding the following code to LoginToutViewController/pushSignupViewController() to present this screeen

fileprivate func pushSignupViewController() {
    let vc = SetYourPasswordViewController.configuredWith(userEmail: "abc*****@***.com")
    self.navigationController?.pushViewController(vc, animated: true)
    self.navigationItem.backBarButtonItem = UIBarButtonItem.back(nil, selector: nil)
  }

• Verify that the screen matches the designs and works in landscape
• Verify that the save button is enabled/disabled appropriately when both passwords match and are at least 5 characters long

⏰ TODO

• Will add tests. I just need to better understand how we do that here first

[msadoon]

Good job!
I left a lot of comments, but the basic autolayout, understanding of Prelude, signal structure, Reactive work is good! You quickly got the hang of it!

Aside from the code comments, I found some things that I would kind of double check to make sure are "tighter" before releasing this to production.

First thing is just a compare/constrast with existing designs looks like the Font sizes, colours, textfield heights, buttons colours might need a little tweaking.

Here's a navigation bar issue:

Simulator.Screen.Recording.-.iPhone.8.-.2022-08-24.at.15.41.47.mp4

As a general guide to supporting screens shown as modals on iPhone and fullscreen on iPad, take a look at this transition from "RewardsCollectionViewController" to "PledgeViewController". Notice that the transition supports the back button like this screen.

Simulator.Screen.Recording.-.iPhone.13.Pro.Max.-.2022-08-24.at.15.53.37.mp4

And then finally the "Done" button could use some love.

Simulator.Screen.Recording.-.iPhone.13.Pro.Max.-.2022-08-24.at.16.27.13.mp4

Some general things:

• Ensure branch is updated with main

• Try to include images like this in PRs (just so they don't blow up in size) "<img src="url" width="300”>"

• I know you didn't include the push here, and technically this is safe to main because it doesn't have any entry points, but if you ever want to change existing feature logic we need to create a Optimizely flag and gate it first before adding feature code. It's just for future stuff, but ask PM/Manager how to get access to Optimizely and create a flag for this feature. I don't know why I can't add you.

• Protip: bin/format.sh . in terminal to autoformat everything before pushing or the CI linter will scream at you. (You might already be doing this!)

Good stuff though, looking forward to the revisions!

[scottkicks]

@msadoon thanks for the feedback. I think I've covered all of your comments. Once we sync on testing I'll get that taken care of as well.

[codecov]

Codecov Report

Merging #1714 (a5d68f7) into main (423abaa) will increase coverage by 0.00%.
The diff coverage is 86.66%.

@@           Coverage Diff            @@
##             main    #1714    +/-   ##
========================================
  Coverage   85.33%   85.33%            
========================================
  Files        1271     1275     +4     
  Lines      114511   114765   +254     
  Branches    30236    30321    +85     
========================================
+ Hits        97715    97937   +222     
- Misses      15746    15778    +32     
  Partials     1050     1050            

Impacted Files	Coverage Δ
...OS/Views/Controllers/LoginToutViewController.swift	58.67% <ø> (-0.12%)	⬇️
Library/ViewModels/SetYourPasswordViewModel.swift	77.77% <77.77%> (ø)
...ws/Controllers/SetYourPasswordViewController.swift	85.80% <85.80%> (ø)
...ntrollers/SetYourPasswordViewControllerTests.swift	100.00% <100.00%> (ø)
...ary/ViewModels/SetYourPasswordViewModelTests.swift	100.00% <100.00%> (ø)
Library/Navigation.swift	85.85% <0.00%> (+0.64%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[scottkicks]

@msadoon Fixed the navbar issue and added tests. The VC test snapshots aren't showing the copy that's on that screen though for some reason. Maybe we can look at that quick when we sync later

[msadoon]

Still a few remaining changes, but we're close!
Good job on navigation bar fix, and the mocks look much similar now (also checked with Lillit, the designs use an iPhone 11 Pro res so best to compare to that simulator)

Here's what I got:

Can you maybe adjust the text labels above both textfields to be smaller font size?

[msadoon]

So yea for the view controller and view model, try to put them in alphabetical order if possible.

[scottkicks]

so the ViewController and ViewModel files are already separate folders and are in alphabetical order

[msadoon]

I just meant that in the directory listing the vc and vm are at the bottom, maybe in your next pr just place them in alpha order like the other files.

[scottkicks]

@msadoon Ok i'll make the textfield labels a touch smaller. That will stray away from what the designs specify though. Also one thing to note, the copy in the designs as changed slightly to what you see in this build. I can't update the designs so that's why that's different.

[msadoon]

@msadoon Ok i'll make the textfield labels a touch smaller. That will stray away from what the designs specify though. Also one thing to note, the copy in the designs as changed slightly to what you see in this build. I can't update the designs so that's why that's different.

Well it looks like the designs have 16px for the context label and 13 px for the text field labels, just set the text field labels to be 13 px. and the context label to be 16px.

[scottkicks]

Oh yea you're right 🤔 . Could have sworn I was seeing other values. My mistake. Will update

[msadoon]

good to go!