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

PM-10278 PM-10279 PM-10806 - Setup autofill screen for new onboarding flow #979

Merged
merged 18 commits into from
Oct 1, 2024

Conversation

phil-livefront
Copy link
Collaborator

@phil-livefront phil-livefront commented Sep 26, 2024

🎟️ Tracking

PM-10278 - Draw Turn on Autofill Setup Screen
PM-10279 - Implement Turn on Autofill Later Confirmation Dialog
PM-10806 - Implement Autofill Help Link

📔 Objective

  • Setup the new PasswordAutoFillView so it can be used in the new onboarding flow and also from within the Settings menu. Also kept the legacy version around and is configured based on the feature flag native-create-account-flow
  • Added a gifView using a web view to render the gif. This allows for first party support and seemed simple enough for the one-off gif. If you think we should intro a 3rd party for this, I am happy to update but this felt better imo.
  • When navigating through the settings mode, kept the deep link to passwords but got rid of the turn on later button
  • Added some parameters to the fixedSized helper function to help with testing
  • Fixed a linter warning for line length on CompleteRegistrationProcessor
  • Used the link out navigation that is used elsewhere in the app. The Figma docs tho show an in-app web view so I kept the behavior as is since that could be a bigger change.

📸 Screenshots

Simulating a email verification link click via Reminders app

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-26.at.17.54.16.mp4

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link
Contributor

github-actions bot commented Sep 26, 2024

Logo
Checkmarx One – Scan Summary & Detailsa909f9e4-f71d-4915-8786-981f8b2fe958

No New Or Fixed Issues Found

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 95.81590% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.97%. Comparing base (d94c5a3) to head (2aa2947).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...toFill/PasswordAutoFill/PasswordAutoFillView.swift 91.93% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #979      +/-   ##
==========================================
+ Coverage   88.91%   88.97%   +0.05%     
==========================================
  Files         647      650       +3     
  Lines       40559    40805     +246     
==========================================
+ Hits        36064    36306     +242     
- Misses       4495     4499       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator Author

@phil-livefront phil-livefront Sep 27, 2024

Choose a reason for hiding this comment

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

I added a placeholder image to display while the GIF loads. This ensures there is always something visible, which is especially helpful for snapshots since the GIF wasn't loading quickly enough before the snapshot was taken. The placeholder is the first frame of the GIF. Happy to remove but this felt like a good call.

Copy link
Member

Choose a reason for hiding this comment

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

Nice! I like having a placeholder, please confirm with Product to see if that image is ok or if they'd prefer another one.

Comment on lines +983 to +991
"FromYourDeviceSettingsToggleOnAutoFillPasswordsAndPasskeys" = "From your device settings, **toggle on autofill Passwords and Passkeys.**";
"ToggleOffICloudToMakeBitwardenYourDefaultAutoFillSource" = "**Toggle off iCloud Keychain** to make Bitwarden your default autofill source.";
"ToggleOnBitwardenToUseYourSavedPasswordsToLogIntoYourAccounts" = "**Toggle on Bitwarden** to use your saved passwords to log into your accounts.";
"TurnOnAutoFill"= "Turn on autofill";
"UseAutoFillToLogIntoYourAccountsWithASingleTap" = "Use autofill to log into your accounts with a single tap.";
"NeedHelpCheckOutAutofillHelp" = "Need help? Check out **[autofill help](%1$@)**";
"TurnOnLater" = "Turn on later";
"TurnOnAutoFillLaterQuestion" = "Turn on autofill later?";
"YouCanReturnToCompleteThisStepAnytimeInSettings" = "You can return to complete this step anytime in Settings.";
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Could you confirm with product on the use of autofill string instead of auto-fill? I see there are a lot of resources using auto-fill so wanted to check if this changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call out. I just confirmed with the livefront designer that "autofill" is correct but lmk if this is something we should escalate up to the BW team?

Comment on lines 47 to 56
ZStack {
gifViewPlaceholder
.frame(width: 230, height: 278)
.background(.red)
.padding(.top, 32)

gifView
.frame(width: 230, height: 280)
.padding(.top, 32)
}
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Does the gif gets displayed correctly on a really small device like iPhone 6s or iPhone SE? What about landscape mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it does! I am going to do some work here on this ticket to have it match the figma designs so I can make sure everything is good on that PR if thats cool with you?

Copy link
Member

Choose a reason for hiding this comment

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

Nice! I like having a placeholder, please confirm with Product to see if that image is ok or if they'd prefer another one.

.padding(.top, 32)

Button(Localizations.continue) {
openURL(ExternalLinksConstants.passwordOptions)
Copy link
Member

Choose a reason for hiding this comment

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

ℹ️ On iOS 17+ there's a more direct way to go there if I'm understanding correctly where this should be headed with the ASSettingsHelper

Copy link
Collaborator

@matt-livefront matt-livefront left a comment

Choose a reason for hiding this comment

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

👍

@phil-livefront phil-livefront merged commit 59c7e1a into main Oct 1, 2024
9 checks passed
@phil-livefront phil-livefront deleted the phil/PM-10278-PM-10279-PM-10806-autofill-view branch October 1, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants