Skip to content

[WEB-696] FacebookResetPassword Improvements #1765

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

Merged
merged 9 commits into from
Jan 17, 2023

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Jan 6, 2023

📲 What

Making some improvements to the ResetYourFacebookPassword screen that was added in #1763

🤔 Why

Event though the original code works and is mostly clean there are some improvements that can be made in order to conform to KS best practices and account for some accessibility edge cases

Feedback being addressed can be found here

🛠 How

  • Some general file renaming and code style clean ups.
  • ResetYourFacebookPasswordViewController snapshots have been removed to avoid the M1 related issues we're seeing with generating snapshots.
  • Ensures the scrollview is not hidden by the keyboard when it is active.
  • Set the keyboard's return key to go and hook up to the on submit api call

New Behavior

Simulator Screen Recording - iPhone 13 Pro - 2023-01-09 at 09 55 04

❗️Important Notes❗️

  • Due to the ongoing circleci failures we're merging into a new branch circleci/pipeline-queue for the time being
  • Please make sure to run tests locally to ensure that nothing is breaking since we can't rely on circleci to check them

@scottkicks scottkicks self-assigned this Jan 6, 2023
@scottkicks scottkicks added this to the release-5.7.0 milestone Jan 6, 2023
@scottkicks scottkicks changed the base branch from main to circleci/pipeline-queue January 11, 2023 15:21
@scottkicks scottkicks marked this pull request as ready for review January 11, 2023 15:24
@scottkicks scottkicks requested review from msadoon and removed request for msadoon January 11, 2023 15:24
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #1765 (926d181) into circleci/pipeline-queue (cd6fe2b) will decrease coverage by 0.00%.
The diff coverage is 67.53%.

❗ Current head 926d181 differs from pull request most recent head ce5a8d7. Consider uploading reports for the commit ce5a8d7 to get more accurate results

@@                     Coverage Diff                     @@
##           circleci/pipeline-queue    #1765      +/-   ##
===========================================================
- Coverage                    85.29%   85.29%   -0.01%     
===========================================================
  Files                         1276     1276              
  Lines                       116541   116567      +26     
  Branches                     30722    30725       +3     
===========================================================
+ Hits                         99408    99423      +15     
- Misses                       16066    16074       +8     
- Partials                      1067     1070       +3     
Impacted Files Coverage Δ
...ntroller/FacebookResetPasswordViewController.swift 0.00% <0.00%> (ø)
...LoginTout/Controller/LoginToutViewController.swift 53.50% <0.00%> (ø)
...ry/ViewModels/FacebookResetPasswordViewModel.swift 98.21% <94.44%> (ø)
...ewModels/FacebookResetPasswordViewModelTests.swift 97.93% <94.59%> (ø)
Library/Navigation.swift 85.20% <0.00%> (-0.65%) ⬇️

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

@msadoon msadoon self-assigned this Jan 16, 2023
@msadoon msadoon modified the milestones: release-5.7.0, release-5.6.1 Jan 16, 2023
@msadoon msadoon self-requested a review January 16, 2023 21:56
Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Good to go!
Added some commits to round out your work, hope you don't mind. I just wanted to get this PR ready to merge as I know you have a lot on your plate and its' just some minor accessibility stuff.

@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@scottkicks scottkicks merged commit a1857a4 into circleci/pipeline-queue Jan 17, 2023
@scottkicks scottkicks deleted the web-696-improvements branch January 17, 2023 15:20
scottkicks added a commit that referenced this pull request Jan 24, 2023
* [WEB-861] Consent Management feature flag (#1764)

* Add new Consent Management Dialog feature flag

* Add new Consent Management Dialog feature flag

* remove failing view controller test assertion

* formatting

* Adding a small commit so tests pass on CI.

Didn't realize that if we removed the feature from the mock client, the recording would would still have it in the snapshot and our old snapshot wouldn't have been updated yet.

* formatting

Co-authored-by: Mubarak Sadoon <[email protected]>

* [WEB-696] FacebookResetPassword Improvements (#1765)

* remove ResetYourFacebookPasswordViewController snapshots

* rename viewmodel and viewcontroller

* viewmodel form logic suggestions

* update viewmodel tests

* ensure the scrollview is not hidden by the keyboard

* set returnKeyType to go and make api call on tap

* update accessibility on set your password view controller

* updated accessibility for facebookresetpasswordviewcontroller.

* formatting

Co-authored-by: Mubarak Sadoon <[email protected]>

* [WEB-669] Facebook Conversions API Feature Flag (#1766)

* add facebook conversions api feature flag

* add facebook conversions api feature flag

* fix test

* [WEB-862] AppTrackingTransparency Authorization (#1772)

* set NSUserTrackingUsageDescription in plist

* request ATTrackingAuthorization on app applicationdidFinishLaunching

* formatting

* gate behind consent management dialog feature flag

* pr feedback
* ATTrackingAuthorizationStatus to its own file
* Use `.ksr_debounce` on Signal instead of `asyncAfter`
* Handle `restricted` and `@unknown` requestTrackingAuthorization status cases
* Improve unit test

* use ksr_delay insted of ksr_debounce

Co-authored-by: Mubarak Sadoon <[email protected]>
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.

3 participants