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

Move Top Level Settings to SwiftUI + Other improvements #2255

Closed
wants to merge 0 commits into from

Conversation

afterxleep
Copy link
Collaborator

@afterxleep afterxleep commented Dec 11, 2023

Task/Issue URL: https://app.asana.com/0/1204099484721401/1206108402009442

Description

Getting rid of the old and ugly SettingsViewController and the related Static Storyboard View, alongside a few other changes as follows:

  1. Remove SettingsViewController and a bunch of needed ViewControllers
  2. Removed Storyboard views for the Top-level settings
  3. Implement Top level settings using SwiftUI + MVVM approach pattern
  4. Add pre-made reusable cell components for displaying standard cells, plus a "Custom" cell that accepts any view.
  5. Legacy settings views (Remaining in the Settings. storyboard) are still rendered via a SwiftUI NavigationViewController to prevent breakage of theme and functionality
  6. Moved strings to UserText and updated translations for all languages
  7. Use DesignResourcesKit to render Settings

How to test

  • Open Settings
  • Validate that all top-level switches are working
  • Validate that secondary views are presented when required, and changes are reflected in the Settings View
  • Check Fonts and colors for oddities.
  • Validate Hidden features are hidden (Internal features not shown to users. i.e Network protection and Sync)

Copy link

github-actions bot commented Dec 11, 2023

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 43c877e

@afterxleep afterxleep marked this pull request as ready for review December 11, 2023 14:38
@afterxleep afterxleep requested review from a team and diegoreymendez and removed request for a team December 11, 2023 18:53
@miasma13 miasma13 requested review from miasma13 and removed request for diegoreymendez December 12, 2023 10:42
@miasma13 miasma13 self-assigned this Dec 12, 2023
Copy link
Contributor

@miasma13 miasma13 left a comment

Choose a reason for hiding this comment

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

Great work, I really appreciate your diligence in the approach.
Please take a look at the comments, nothing critical, couple of typos. Besides these I would also:

  1. Check and fix indentation and empty lines (especially in the files for respective sections)
  2. Perform (if you haven't) more in-depth testing for older iOS versions: iOS 16, iOS 15 and iOS 14 (especially if we will merge before dropping this version to make sure we won't introduce anything breaking in the last version for 14).

DuckDuckGo/FontSettings.swift Outdated Show resolved Hide resolved
DuckDuckGo/MainViewController+Segues.swift Outdated Show resolved Hide resolved
DuckDuckGo/SettingsCell.swift Outdated Show resolved Hide resolved
DuckDuckGo/SettingsHostingController.swift Outdated Show resolved Hide resolved
DuckDuckGo/UserText.swift Outdated Show resolved Hide resolved
DuckDuckGo/UserText.swift Outdated Show resolved Hide resolved
DuckDuckGo/SettingsViewModel.swift Outdated Show resolved Hide resolved
DuckDuckGo/SettingsDebugView.swift Outdated Show resolved Hide resolved
@miasma13 miasma13 assigned afterxleep and unassigned miasma13 Dec 22, 2023
@afterxleep afterxleep requested a review from miasma13 January 4, 2024 12:00
@afterxleep
Copy link
Collaborator Author

@miasma13 I’ve addressed your feedback, but I’m not sure why you’ve blocked the PR given suggestions are about a few typos and code style. Is there anything else I missed?

@afterxleep afterxleep closed this Jan 5, 2024
@afterxleep afterxleep force-pushed the daniel/settings-swiftui branch from 43c877e to f59497d Compare January 5, 2024 16:28
@afterxleep
Copy link
Collaborator Author

@miasma13 Keeping this open and up to date is taking a lot of time. I've merged these changes in the first subscription PR, which is ready for review here: #2259

@afterxleep afterxleep deleted the daniel/settings-swiftui branch April 11, 2024 18:36
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.

2 participants