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

[NTV-613] Change Email View SwiftUI #1803

Merged
merged 31 commits into from
Mar 27, 2023

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Mar 20, 2023

📲 What

As part of a personal goal to integrate SwiftUI into the main iOS Kickstarter repo, here is the prototype PR to change this view over to SwiftUI with some Combine.

🤔 Why

SwiftUI has been around for about 4 years now from Apple and will help us quickly iterate new views, add new functionality, refactor out xibs, storyboards and programmatic views, reduce app size, add accessibillity and increase developer efficiency.

It will also help us reduce dependencies like Prelude which allow us to modify our view properties. SwiftUI comes with built in view modifiers of its own.

It’s generally more versatile as it supports multiple platforms like MacOS, iPadOS, iOS out of the box. It's much easier to deal with storyboards, constraints, autolayout for different screen sizes like iPad.

One of its biggest benefits is it's built to be state based view rendering with Combine.

Combine is library much like our existing ReactiveSwift and ReactiveExtension frameworks for streaming data changes through the app. It’s a subscriber/publisher data flow that can render views based on data state. The similarity here between Combine and ReactiveSwit is a convenient spot to overhaul some of the business logic as well.

With all these benefits and close to no drawbacks (except learning curve), why not go ahead and convert an existing page over to SwiftUI and Combine.

🛠 How

As you can see all the new code is in ChangeEmailView and represents most of the displayed UI within this page.
It's also launching inside a UIHostingViewController and works on iOS 15 plus.

So generally the approach was to keep as much ReactiveSwift as possible which meant copying the old ChangeEmailViewModel and renaming it ChangeEmailViewModel_SwiftUIIntegrationTest.

Once the ui was complete (ChangeEmailView) we created an instance of the copied view model and made it an ObservableObject.

Without getting into the hairy details too much, the general idea was to use Combine's Published properties to be bound to the SwiftUI views through the ObservedObject (the vm).

We also use PassthroughSubject, AnyPublisher and CurrentValueSubject to update the view from the view model. The onReceive view modifier accepts the subjects from the view model and updates a local @State property that is bound to the view. onChange goes the other way by updating the view model from @State properties in the view.

The nice thing about this approach is its' simplicity because we start with our old view model in ReactiveSwift and migrate over Signals to Publisher or Subject and inputs MutableProperty become PassthroughSubject and CurrentValueSubject.

This is probably a messier implementation than what is "best practices", but its' a good start in that the feature is 95% as functional as the older UIKit version.

Also converted over the LoadingBarButtonItemView to LoadingBarButtonItem and MessageBannerViewController, MessageBannerViewModel to MessageBannerView MessageBannerViewViewModel

An RFC will be written later to discuss total developer efficiency, once we completely remove the ReactiveSwift from this view and make it testable. Effectively retiring our ChangeEmailViewController and its' dependencies.

Some caveats

  • The message banner view is not perfectly sized around the text and can sometimes (on landscape) cut off long text on smaller phone sizes (SE)
  • The "Done" button doesn't autotrigger save as it previously did.

Note: Did notice that the resend verification email text is not being sent to my phone number at least (even on production).

👀 See

UIKit 🐛

RPReplay_Final1679516155.MP4

SwiftUI 🦋

RPReplay_Final1679516394.MP4

✅ Acceptance criteria

  • ChangeEmailView replaces ChangeEmailViewController for iOS 15.0+
    - [x] User can change email successfully.
    - [x] If change email errors or succeeds states on view are updated so user can try again.

⏰ TODO

  • Ensure when save is complete, we update the save button.
  • Ensure both textfields are disabled while saving (ie. loading state == true)
  • After successful save then clear the new email and password fields
  • subsequent saves not working because save is not enabled
  • subsequent saves not showing error/success banner message.
  • validate that verification email view is hidden after email is verified.
  • fix backspace issue triggering save.
  • Ensure entire message for banner shows on save/error. Should show three lines...but as long as entire message shows its' fine.

Still missing navigation bar content, but it is launching inside UIHostingViewController, so that's good news!
… of tinkering to reconfigure the ChangelEmailView to contain an instance of MessageBannerView.
…mapping:

- voice over and message banner view hide, banner message accessibility label. Also add animation and size change on orientation change. Also double check dynamic type and voice over when complete.
… to use Combine instead of ReactiveSwift so its more complicated that first planned.
TODO:
1. Ensure when save is complete, we update the save button.
2. Ensure both textfields are disabled while saving (ie. loading state == true)
3. After sucessful save then clear the new email and password fields
@msadoon msadoon self-assigned this Mar 20, 2023
@msadoon msadoon added the WIP label Mar 20, 2023
@msadoon msadoon added this to the release-5.7.0 milestone Mar 20, 2023
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #1803 (bcea10f) into main (284ef75) will decrease coverage by 0.31%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1803      +/-   ##
==========================================
- Coverage   85.35%   85.05%   -0.31%     
==========================================
  Files        1286     1292       +6     
  Lines      117942   118359     +417     
  Branches    31232    31350     +118     
==========================================
- Hits       100675   100673       -2     
- Misses      16181    16600     +419     
  Partials     1086     1086              
Impacted Files Coverage Δ
...tures/ChangeEmail/Controller/ChangeEmailView.swift 0.00% <0.00%> (ø)
...atures/MessageBanner/Views/MessageBannerView.swift 0.00% <0.00%> (ø)
...unt/Controller/SettingsAccountViewController.swift 68.00% <0.00%> (-2.25%) ⬇️
...starter-iOS/SharedViews/LoadingBarButtonItem.swift 0.00% <0.00%> (ø)
...dViews/ViewModifiers/TextInputFieldModifiers.swift 0.00% <0.00%> (ø)
Library/Styles/Colors.swift 40.00% <0.00%> (-1.67%) ⬇️
.../ChangeEmailViewModel_SwiftUIIntegrationTest.swift 0.00% <0.00%> (ø)
...ibrary/ViewModels/MessageBannerViewViewModel.swift 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

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

…s/errors don't show banner message.

- subsequent saves not working because save is not enabled
- subsequent saves not showing erorr/success banner message.
- validate that verification email view is hidden after email is verified.
- fix backspace issue triggering save.
@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@msadoon msadoon marked this pull request as ready for review March 22, 2023 20:59
@msadoon msadoon requested a review from scottkicks March 22, 2023 20:59
@msadoon
Copy link
Contributor Author

msadoon commented Mar 27, 2023

@scottkicks gonna merge this to start the release -- as you are OOO. Hope you don't mind, normally your review is required. Figured though we'll regression test this page alongside QA parties so it'll probably be OK. Also when you have time feel free to review this PR and suggest any improvements even if its' merged.

@msadoon msadoon merged commit ad7b971 into main Mar 27, 2023
@msadoon msadoon deleted the prototype/swiftui-change-email-almost-complete branch March 27, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants