Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import UIKit

struct MeHeaderViewConfiguration {

let gravatarEmail: String?
let username: String
let displayName: String
}

extension MeHeaderViewConfiguration {

init(account: WPAccount) {
self.init(
gravatarEmail: account.email,
username: account.username,
displayName: account.displayName
)
}
}

extension MeHeaderView {

func update(with configuration: Configuration) {
self.gravatarEmail = configuration.gravatarEmail
self.username = configuration.username
self.displayName = configuration.displayName
}

typealias Configuration = MeHeaderViewConfiguration
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ extension UITableView {
withHorizontalFittingPriority: .required,
verticalFittingPriority: .fittingSizeLevel
)
tableHeaderView.frame = CGRect(origin: .zero, size: size)
self.tableHeaderView = tableHeaderView
let newFrame = CGRect(origin: .zero, size: size)
if tableHeaderView.frame.height != newFrame.height {
tableHeaderView.frame = newFrame
self.tableHeaderView = tableHeaderView
}
}
}
38 changes: 38 additions & 0 deletions WordPress/Classes/ViewRelated/Support/LogOutActionHandler.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import UIKit

struct LogOutActionHandler {

func logOut(with viewController: UIViewController) {
let alert = UIAlertController(title: logOutAlertTitle, message: nil, preferredStyle: .alert)
alert.addActionWithTitle(Strings.alertCancelAction, style: .cancel)
alert.addActionWithTitle(Strings.alertLogoutAction, style: .destructive) { [weak viewController] _ in
viewController?.dismiss(animated: true) {
AccountHelper.logOutDefaultWordPressComAccount()
}
}
viewController.present(alert, animated: true)
}

private var logOutAlertTitle: String {
let context = ContextManager.sharedInstance().mainContext
let count = AbstractPost.countLocalPosts(in: context)

guard count > 0 else {
return Strings.alertDefaultTitle
}

let format = count > 1 ? Strings.alertUnsavedTitlePlural : Strings.alertUnsavedTitleSingular
return String(format: format, count)
}


private struct Strings {
static let alertDefaultTitle = AppConstants.Logout.alertTitle
static let alertUnsavedTitleSingular = NSLocalizedString("You have changes to %d post that hasn't been uploaded to your site. Logging out now will delete those changes. Log out anyway?",
comment: "Warning displayed before logging out. The %d placeholder will contain the number of local posts (SINGULAR!)")
static let alertUnsavedTitlePlural = NSLocalizedString("You have changes to %d posts that haven’t been uploaded to your site. Logging out now will delete those changes. Log out anyway?",
comment: "Warning displayed before logging out. The %d placeholder will contain the number of local posts (PLURAL!)")
static let alertCancelAction = NSLocalizedString("Cancel", comment: "Verb. A button title. Tapping cancels an action.")
static let alertLogoutAction = NSLocalizedString("Log Out", comment: "Button for confirming logging out from WordPress.com account")
}
Comment on lines +29 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the new reverse dns notation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the log out logic is already implemented in MeViewController, I re-used the same logic for the support screen, so those strings aren't new. I think if we change the keys, then translators will have to provide a new translations for those strings, right? If so, is there way to change the keys without losing the existing translation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It's fine for me to keep them as they are, just pinging @AliSoftware since I saw a recent issue with localized strings, to make sure this won't cause any

Copy link
Contributor

@AliSoftware AliSoftware Nov 18, 2022

Choose a reason for hiding this comment

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

Thanks for the ping!

Using the exact same strings as the MeViewController is fine… as long as you are really sure that they are used in the same kind of UI context (and that they should thus have the exact same translations in both places, because they really are about the exact same thing)

  • That being said, instead of copy/pasting the strings and comments in there, and thus duplicating the declaration of those NSLocalizedStrings (between the ones in the MeViewController and the ones here) — which could lead to a later PR changing the comment here, but not in the other place, and lead to more confusion / discrepancies by not having a single source of truth for those code declarations), I'd instead suggest to reference the existing constants from MeViewController (or move those constants into a common class / file / ViewModel / what have you, as long as you declare them only once and reference them from both places)
  • This might also be a more "dangerous" assumption to make for shorter copies (like "Cancel" and "Log Out") which seems like words that are small and common enough to be used in many other places in the app. So if another place in the app has NSLocalizedString("Cancel", comment: "Some completely different context"), maybe the translators would be tempted to translate it differently for one context than for the other, but if you use the same key for too many different contexts and places in the code, that might lead to translation inconsistencies, like what happened here in the past.

The last point and example is one more key reason why semantic keys are helpful and that it could still be a good idea to switch to them here 😉

If so, is there way to change the keys without losing the existing translation?

Not automatically… but almost.

It happens that GlotPress has a system they call "translation memory", and when it detects a source (English) copy for which they already had a translation for in the past, even if for a different key,… then it is automatically suggested to translators:

image

See how this recent key in GlotPress is not translated yet… but the "Suggestions from Translation Memory" section already have translations for it, one click away to be used via the "Copy" buttons

This means that, yes, if you change the key in this NSLocalizedString call to start using semantic keys here instead, the translation for that new key will start as empty / untranslated the first time that new key will be imported into GlotPress… but then it will be only one click away for polyglots to translate (picking if from the suggested translation from Translation Memory) as opposed to having to re-translate it from scratch. So that new key will get the existing translation from the old key pretty quickly, so I'd say in the end that's not that big a deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying @AliSoftware 🙇

I'd instead suggest to reference the existing constants from MeViewController (or move those constants into a common class / file / ViewModel / what have you, as long as you declare them only once and reference them from both places)

I totally agree with this and the log out logic duplication is temporary. Right now the type LogOutActionHandler is used only in SupportTableViewController but I'll open another PR so that MeViewController uses it as well. As a result, those strings declaration will no longer be duplicated.

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ class SupportTableViewController: UITableViewController {

// MARK: - Properties

/// Configures the appearance of the support screen.
let configuration: Configuration

var sourceTag: WordPressSupportSourceTag?

// If set, the Zendesk views will be shown from this view instead of in the navigation controller.
Expand All @@ -21,7 +24,8 @@ class SupportTableViewController: UITableViewController {

// MARK: - Init

override init(style: UITableView.Style) {
init(configuration: Configuration = .init(), style: UITableView.Style = .grouped) {
self.configuration = configuration
super.init(style: style)
}

Expand All @@ -46,6 +50,11 @@ class SupportTableViewController: UITableViewController {
ZendeskUtils.fetchUserInformation()
}

override func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()
self.tableView.sizeToFitHeaderView()
}

override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)
reloadViewModel()
Expand Down Expand Up @@ -78,7 +87,6 @@ class SupportTableViewController: UITableViewController {
dismissTapped?()
dismiss(animated: true)
}

}

// MARK: - Private Extension
Expand Down Expand Up @@ -107,14 +115,18 @@ private extension SupportTableViewController {
NavigationItemRow.self,
TextRow.self,
HelpRow.self,
DestructiveButtonRow.self,
SupportEmailRow.self],
tableView: tableView)
tableHandler = ImmuTableViewHandler(takeOver: self)
reloadViewModel()
WPStyleGuide.configureColors(view: view, tableView: tableView)
// remove empty cells
tableView.tableFooterView = UIView()

tableView.tableFooterView = UIView() // remove empty cells
if let headerConfig = configuration.meHeaderConfiguration {
let headerView = MeHeaderView()
headerView.update(with: headerConfig)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, the MeHeaderView should be initialized with the config param, but the MeHeaderView is used elsewhere and would require some refactoring to be done.

tableView.tableHeaderView = headerView
}
registerObservers()
}

Expand Down Expand Up @@ -144,19 +156,34 @@ private extension SupportTableViewController {
footerText: LocalizedText.helpFooter)

// Information Section
let versionRow = TextRow(title: LocalizedText.version, value: Bundle.main.shortVersionString())
let switchRow = SwitchRow(title: LocalizedText.extraDebug,
value: userDefaults.bool(forKey: UserDefaultsKeys.extraDebug),
onChange: extraDebugToggled())
let logsRow = NavigationItemRow(title: LocalizedText.activityLogs, action: activityLogsSelected(), accessibilityIdentifier: "activity-logs-button")
var informationSection: ImmuTableSection?
if configuration.showsLogsSection {
let versionRow = TextRow(title: LocalizedText.version, value: Bundle.main.shortVersionString())
let switchRow = SwitchRow(title: LocalizedText.extraDebug,
value: userDefaults.bool(forKey: UserDefaultsKeys.extraDebug),
onChange: extraDebugToggled())
let logsRow = NavigationItemRow(title: LocalizedText.activityLogs, action: activityLogsSelected(), accessibilityIdentifier: "activity-logs-button")
informationSection = ImmuTableSection(
headerText: nil,
rows: [versionRow, switchRow, logsRow],
footerText: LocalizedText.informationFooter
)
}

let informationSection = ImmuTableSection(
headerText: nil,
rows: [versionRow, switchRow, logsRow],
footerText: LocalizedText.informationFooter)
// Log out Section
var logOutSections: ImmuTableSection?
if configuration.showsLogOutButton {
let logOutRow = DestructiveButtonRow(
title: LocalizedText.logOutButtonTitle,
action: logOutTapped(),
accessibilityIdentifier: ""
)
logOutSections = .init(headerText: LocalizedText.wpAccount, optionalRows: [logOutRow])
}

// Create and return table
return ImmuTable(sections: [helpSection, informationSection])
let sections = [helpSection, informationSection, logOutSections].compactMap { $0 }
return ImmuTable(sections: sections)
}

@objc func refreshNotificationIndicator(_ notification: Foundation.Notification) {
Expand Down Expand Up @@ -274,6 +301,17 @@ private extension SupportTableViewController {
}
}

private func logOutTapped() -> ImmuTableAction {
return { [weak self] row in
guard let self else {
return
}
self.tableView.deselectSelectedRowWithAnimation(true)
let actionHandler = LogOutActionHandler()
actionHandler.logOut(with: self)
}
}

// MARK: - ImmuTableRow Struct

struct HelpRow: ImmuTableRow {
Expand Down Expand Up @@ -345,6 +383,8 @@ private extension SupportTableViewController {
static let contactEmail = NSLocalizedString("Contact Email", comment: "Support email label.")
static let contactEmailAccessibilityHint = NSLocalizedString("Shows a dialog for changing the Contact Email.", comment: "Accessibility hint describing what happens if the Contact Email button is tapped.")
static let emailNotSet = NSLocalizedString("Not Set", comment: "Display value for Support email field if there is no user email address.")
static let wpAccount = NSLocalizedString("WordPress.com Account", comment: "WordPress.com sign-out section header title")
static let logOutButtonTitle = NSLocalizedString("Log Out", comment: "Button for confirming logging out from WordPress.com account")
Comment on lines +386 to +387
Copy link
Contributor

Choose a reason for hiding this comment

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

same consideration here about the strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as this reply

}

// MARK: - User Defaults Keys
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import Foundation

struct SupportTableViewControllerConfiguration {

// MARK: Properties

var meHeaderConfiguration: MeHeaderView.Configuration?
var showsLogOutButton: Bool = false
var showsLogsSection: Bool = true

// MARK: Default Configurations

static func currentAccountConfiguration() -> Self {
var config = Self.init()
if let account = Self.makeAccount() {
config.meHeaderConfiguration = .init(account: account)
config.showsLogOutButton = true
config.showsLogsSection = false
}
return config
}

private static func makeAccount() -> WPAccount? {
let context = ContextManager.shared.mainContext
do {
return try WPAccount.lookupDefaultWordPressComAccount(in: context)
} catch {
DDLogError("Account lookup failed with error: \(error)")
return nil
}
}
}

extension SupportTableViewController {

typealias Configuration = SupportTableViewControllerConfiguration
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,30 @@ struct MigrationViewControllerFactory {
}
}

private func makeWelcomeViewModel() -> MigrationWelcomeViewModel {
MigrationWelcomeViewModel(account: makeAccount(), coordinator: coordinator)
// MARK: - View Controllers

private func makeWelcomeViewModel(handlers: ActionHandlers) -> MigrationWelcomeViewModel {
let primaryHandler = { () -> Void in handlers.primary?() }
let secondaryHandler = { () -> Void in handlers.secondary?() }

let actions = MigrationActionsViewConfiguration(
step: .welcome,
primaryHandler: primaryHandler,
secondaryHandler: secondaryHandler
)

return .init(account: makeAccount(), actions: actions)
}

private func makeWelcomeViewController() -> UIViewController {
MigrationWelcomeViewController(viewModel: makeWelcomeViewModel())
let handlers = ActionHandlers()
let viewModel = makeWelcomeViewModel(handlers: handlers)

let viewController = MigrationWelcomeViewController(viewModel: viewModel)
handlers.primary = { [weak coordinator] in coordinator?.transitionToNextStep() }
handlers.secondary = makeSupportViewControllerRouter(with: viewController)

return viewController
}

private func makeNotificationsViewModel() -> MigrationNotificationsViewModel {
Expand All @@ -67,4 +85,20 @@ struct MigrationViewControllerFactory {
private func makeDoneViewController() -> UIViewController {
MigrationDoneViewController(viewModel: makeDoneViewModel())
}

// MARK: - Routers

private func makeSupportViewControllerRouter(with presenter: UIViewController) -> () -> Void {
return { [weak presenter] in
let destination = SupportTableViewController(configuration: .currentAccountConfiguration(), style: .insetGrouped)
presenter?.present(UINavigationController(rootViewController: destination), animated: true)
}
}

// MARK: - Types

private class ActionHandlers {
var primary: (() -> Void)?
var secondary: (() -> Void)?
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,22 @@ class MigrationNavigationController: UINavigationController {
private var cancellable: AnyCancellable?

override var supportedInterfaceOrientations: UIInterfaceOrientationMask {
if let presentedViewController {
return presentedViewController.supportedInterfaceOrientations
}
if WPDeviceIdentification.isiPhone() {
return .portrait
} else {
return .allButUpsideDown
}
}

// Force portrait orientation for migration view controllers only
override var preferredInterfaceOrientationForPresentation: UIInterfaceOrientation {
.portrait
if let presentedViewController {
return presentedViewController.preferredInterfaceOrientationForPresentation
}
return .portrait
}

init(coordinator: MigrationFlowCoordinator, factory: MigrationViewControllerFactory) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ final class MigrationWelcomeViewController: UIViewController {
}

private func setupNavigationBar() {
self.navigationItem.rightBarButtonItem = UIBarButtonItem(email: viewModel.gravatarEmail)
self.navigationItem.rightBarButtonItem = UIBarButtonItem(email: viewModel.gravatarEmail) { [weak self] () -> Void in
self?.viewModel.configuration.actionsConfiguration.secondaryHandler?()
}
}

private func setupBottomSheet() {
Expand Down
Loading