-
Notifications
You must be signed in to change notification settings - Fork 14
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
onboarding updates #398
onboarding updates #398
Conversation
# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj
…r into brindy/onboarding-v2
… prevent typing when the user comes back to this view after completing the onboarding
# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, no real issues here – ran through all of the new features and it all looks great. Only a couple small things:
- Unit tests are failing
- I definitely see these errors you mentioned, even though the app compiles. A few errors about AppKit types not being available, but clearly they are. Xcode 13.2 issue I guess
- The Import button spacing feels a little cramped. I would expect it to have the same spacing as the title/description text have between each other. That said, if this is consistent with the design then please ignore this 🙂
- We show the Chrome Keychain password copy even if the app does have access to the key (i.e. has been granted permission in the past), meaning that we tell users they'll be prompted but then they aren't. Pretty minor in the grand scheme of things, and to be honest I'm inclined to keep the copy anyway – better to always explain what's happening with user data at sensitive as this.
I also still see the text jumping around in the typing animation but that's a known issue related to having emoji in the strings yeah? I forget where you got to on that issue sorry. |
Addressed (or deliberately ignored) all your points. :) This is ready for another review, please :) |
@bstandaert-ddg any chance you can give this a bit of a hammering if you've got time, please? |
let chars = Array(text) | ||
let untypedChars = chars[Array(typedText).count ..< chars.count] | ||
let combined = NSMutableAttributedString(string: typedText) | ||
combined.append(NSAttributedString(string: String(untypedChars), attributes: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason this just reminded me that there's a new attributed string API in macOS 12. Not really relevant here, but I'm excited to be able to use it one day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes look good. I bashed on the import flow a bunch and tested importing, canceling early, etc. Nothing seems to be blocking onboarding from progressing now, and the changes to the importer all worked for me. 🙂
@brindy If I type about:welcome, once I get to the import step and click "import", the browser crashes: Perhaps related, Xcode is showing me these weird errors: Any ideas? |
Absolutely no idea 😬 Could try nuking your DerivedData folder:
I'm on Xcode 13.2.1 if that matters. Also that screenshot looks unfamiliar - where in the Xcode UI is that from? Finally, I presume you have at least one of those browsers installed? |
Thanks for the suggestion. The Xcode errors are gone now, but it's still crashing when I try to import. Will take a look later and see if I can figure out why. |
The crash I was seeing turned out to be unrelated to this PR. Made an asana task: https://app.asana.com/0/1199178362774117/1201696927820456/f Will get back to testing this shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bashed on the changes to the importer a bit and looked over the design changes, looks good!
I don't see any issues either! One tiny question: If I click "import", then "cancel" from within the dialog, the welcome page moves on to the next step (default browser). Is that expected, or should it stay on the import step until I click "maybe later"? |
Thanks @bstandaert-ddg - it's consistent with the set default one too. Possibly worth creating a follow up task for. |
* develop: Version 0.18.5 support privacy config for clickToLoad (#407) Automatically select available login (#405) initial FB Click to Load (WIP) (#329) onboarding updates (#398) Version 0.18.4 Configuration of Sparkle - Setting SUAllowsAutomaticUpdates to NO (#404) Hide downloads button if the popover is opened/closed manually (#397)
# By Alexey Martemyanov (20) and others # Via Tomas Strba (2) and others * develop: (63 commits) Tweaks of suggestions and autocomplete (#403) Bump privacy dashboard to latest version (#409) Point to the latest BrowserServicesKit branch. (#414) Move embedded TDS from BSK to platform repo (#412) Image of shield with dot replaced (#410) Expand Fireproofing to include Local Storage and IndexedDB (#408) Version 0.18.5 support privacy config for clickToLoad (#407) Automatically select available login (#405) initial FB Click to Load (WIP) (#329) onboarding updates (#398) Version 0.18.4 Configuration of Sparkle - Setting SUAllowsAutomaticUpdates to NO (#404) Hide downloads button if the popover is opened/closed manually (#397) Textfield of the homepage is empty and unfocused right after switching to the homepage (#400) Remove navigatorCredentials (#392) Remove GPC header if it exists when not needed (#366) Version 0.18.3 Fireproofing encrypted storage (#332) Fix Lock Screen UI issues (#399) ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/Crash Reports/Model/CrashReportSender.swift
Task/Issue URL: https://app.asana.com/0/392891325557410/1201320333982195
Tech Design URL:
CC:
Description:
about:welcome
Notes:
Steps to test this PR:
about:welcome
and confirm works as expected in "clean" install scenarioInternal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM