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

Site Settings in SwiftUI: General #20855

Closed
wants to merge 10 commits into from

Conversation

kean
Copy link
Contributor

@kean kean commented Jun 13, 2023

HACK week: update Site Settings to use SwiftUI

Previous PR: #20838

This PR adds all of the fields from the "General" section to the new Site Settings screen.

  • Add the remaining fields to the General section: address, privacy, etc
  • Introduce SettingsPicker to replace SettingsSelectionViewController
  • Add support for copying site URL (new feature requested in Offer ways to quickly copy the site URL #15927)
Screenshot 2023-06-14 at 12 08 00 PM

To test:

  • Add to AppDelegate:
DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) {
    let host = NewSiteSettingsViewController(blog: {
        let context = ContextManager.shared.mainContext
        return Blog.lastUsedOrFirst(in: context)!
    }())

    let nav = UINavigationController(rootViewController: host)
    nav.modalPresentationStyle = .fullScreen
    self.window?.rootViewController?.present(nav, animated: true)
}
  • Change any of the values in the "General" section
  • Test that the values are updated (use a different app or the existing settings screen)
  • Verify that the language picker works the same way as in the production version (make sure to check search!)

Regression Notes

  1. Potential unintended areas of impact: Site Settings screen
  2. What I did to test those areas of impact (or what existing automated tests I relied on): manual tests
  3. What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@kean kean changed the base branch from trunk to feature/settings-swiftui-01 June 13, 2023 18:24
@objc func languageSelector(_ selector: LanguageSelectorViewController, didSelect languageId: Int) {
_ = navigationController?.popToViewController(self, animated: true)
navigationController?.popViewController(animated: true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change doesn't affect any of the functionality because LanguageSelectorViewController doesn't push any screens. The change was needed to make it work with SwiftUI.

@@ -0,0 +1,90 @@
import SwiftUI

struct SettingsPicker<T: Hashable>: View {
Copy link
Contributor Author

@kean kean Jun 13, 2023

Choose a reason for hiding this comment

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

There were multiple reasons why we couldn't use SwiftUI.Picker. For example, it has no support for section footers, or even inset-grouped list style.

Screenshot 2023-06-13 at 2 23 15 PM

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 13, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr20855-8cfd8c8
Version22.6
Bundle IDorg.wordpress.alpha
Commit8cfd8c8
App Center BuildWPiOS - One-Offs #5981
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 13, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr20855-8cfd8c8
Version22.6
Bundle IDcom.jetpack.alpha
Commit8cfd8c8
App Center Buildjetpack-installable-builds #5009
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean kean marked this pull request as draft June 14, 2023 13:08
@kean kean force-pushed the feature/settings-swiftui-02 branch from 0f40ef0 to f25e2ab Compare June 14, 2023 16:00
@@ -3,25 +3,44 @@ import SwiftUI
import Combine
import SVProgressHUD

final class NewSiteSettingsViewController: UIHostingController<SiteSettingsView> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rename it once the SiteSettingsViewController is removed.

// MARK: - Helpers (Timezone)

private func startTimezoneObserver() {
self.timezoneObserver = TimeZoneObserver { [weak self] _, newState in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this code from the old SiteSettingsViewController with almost no changes.

@kean kean added this to the Pending milestone Jun 14, 2023
}
}

struct TimezoneSelectorView: UIViewControllerRepresentable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to replace these screens as well if I have time, but it's at the bottom of the priority list because they are already written in Swift, and there aren't many reasons to replace them.

// The default `UISearchController` behavior, which is to hide the
// navigation bar, was not working properly when presenting using
// `UIViewControllerRepresentable`.
viewController.hidesNavigationBarDuringPresentation = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty much the only limitation I encountered so far.

@kean kean force-pushed the feature/settings-swiftui-02 branch from f25e2ab to 8cfd8c8 Compare June 14, 2023 16:52
@kean kean marked this pull request as ready for review June 14, 2023 18:51
@kean kean removed request for mokagio and momo-ozawa June 14, 2023 19:45
@kean kean removed the request for review from alpavanoglu June 14, 2023 19:45
@kean kean marked this pull request as draft June 14, 2023 19:45
@kean kean marked this pull request as ready for review June 14, 2023 19:58
@kean kean mentioned this pull request Jun 14, 2023
13 tasks
@kean kean closed this Jun 14, 2023
@jkmassel jkmassel deleted the feature/settings-swiftui-02 branch July 26, 2024 18:20
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.

2 participants