♿️ - Settings #546

dusi · 2019-01-07T21:43:08Z

📲 What

Settings accessibility cleanup

🤔 Why

Some of our settings screens needed a bit of a11y love 🙏

🛠 How

More in-depth discussion of the change or implementation.

👀 See

#532 - Dismiss button
#534 - Settings - rows
#535 - Settings - Change email status
#537 - Settings - Change email - Current email
#545 - Settings - Change email / password input fields
#549 - Settings - Account - Privacy - Delete my KSR account button
#550 - Settings - Heading
#552 - Settings - Notifications - Manage project notifications + Email frequency
#547 - Settings - Notifications - Daily Digest picker bug
#544 - Settings - Switch toggle rows
#539 - Settings - Message banner

* Add accessibility label to dismiss button & fix the minimum width * Set navigation item properties using key paths

* Add button traits to settings cells * Invert disclosure indicators in Settings > Help

* Group current email title and value for a better accessibility * Simplify the way we set accessibility label on current email container

* Accessibility - Settings - Dismiss button (#532) * Add accessibility label to dismiss button & fix the minimum width * Set navigation item properties using key paths * Accessibility - Settings - Rows (#534) * Add button traits to settings cells * Invert disclosure indicators in Settings > Help * Add accessibility hint in case email is unverified (#535) * Focuses Voice Over on the banner when presented * Delay banner dismissal a bit more when VoiceOver is ON * Localize banner message

* Accessibility - Settings - Dismiss button (#532) * Add accessibility label to dismiss button & fix the minimum width * Set navigation item properties using key paths * Accessibility - Settings - Rows (#534) * Add button traits to settings cells * Invert disclosure indicators in Settings > Help * Accessibility - Settings - Dismiss button (#532) * Add accessibility label to dismiss button & fix the minimum width * Set navigation item properties using key paths * Accessibility - Settings - Rows (#534) * Add button traits to settings cells * Invert disclosure indicators in Settings > Help * Add accessibility hint in case email is unverified (#535) * Accessibility - Settings - Change email - Current email (#537) * Group current email title and value for a better accessibility * Simplify the way we set accessibility label on current email container * a11y for notification settings * swiftlint fix

* textfield a11y improvement in change email and password screens * change stackview alignment * new snapshots * fixed strings

* Fix newsletter switch cell accessibility * Fix project notifications switch cell accessibility * Fix settings privacy switch cell accessibility * Refocus VoiceOver on the cell when the confirmation dialog disappears * Place UIAccessibility notification behind a VoiceOver running check * Prevent refreshing the following cell when confirming the alert dialogue * Alphabetize properties * Localize properly

* corrected picker view hidden in iphone X in VM * Library tests * Update snapshot tests * vm and snapshot tests * snapshot tests * wip - digest picker is selected * properly aligned tableview constraint * removed output to hide picker * emailPickerViewHeight * VoiceOver re-focuses on the Email frequency cell * adjusted animation added guard * changed comment * Update Kickstarter-iOS/Views/Controllers/SettingsNotificationsViewController.swift Co-Authored-By: cdolm92 <cdolm92@gmail.com> * Apply suggestions from code review comments adjustment Co-Authored-By: cdolm92 <cdolm92@gmail.com> * Update Kickstarter-iOS/Views/Controllers/SettingsNotificationsViewController.swift Co-Authored-By: cdolm92 <cdolm92@gmail.com>

…frequency (#552) * Add button traits to project notifications & email frequency rows * Remove unused accessibility hint * Remove accessibility value

justinswart

Cool, some suggestions!

justinswart · 2019-01-29T19:32:43Z

Kickstarter-iOS/Views/Cells/SettingsFollowCell.swift

@@ -63,6 +76,10 @@ internal final class SettingsFollowCell: UITableViewCell, ValueCell {
    self.followingSwitch.rac.on = self.viewModel.outputs.followingPrivacyOn
  }

+  func toggleOn(animated: Bool = true) {
+    self.followingSwitch.setOn(true, animated: animated)


Why is true hard-coded here 🤔

Good question. So this method could be more generic (i.e. toggle(on: Bool, animated: Bool)) but since we only ever need to toggle back ON we have used more specific function named toggleOn which implies that we're switching the toggle ON...hence setOn(true, ...)

justinswart · 2019-01-29T19:37:36Z

Kickstarter-iOS/Views/Controllers/MessageBannerViewController.swift

+    UIView.animate(
+      withDuration: duration,
+      delay: 0.0,
+      options: UIView.AnimationOptions.curveEaseInOut,


Can this infer .curveEaseInOut or [.curveEaseInOut]?

Hopefully it won't break the compiler

justinswart · 2019-01-29T19:39:28Z

Kickstarter-iOS/Views/Controllers/SettingsNotificationsViewController.swift

+      animations: { [weak self] in
+        guard let self = self else { return }
+
+        if !isHidden {


Should we combine the nested ifs here into one? if !isHidden && AppEnvironment.current.isVoiceOverRunning(), also on line 115.

Kickstarter-iOS/Views/Controllers/SettingsViewController.swift

justinswart · 2019-01-29T19:53:40Z

Library/ViewModels/MessageBannerViewModel.swift

@@ -53,14 +54,34 @@ MessageBannerViewModelInputs, MessageBannerViewModelOutputs {
    self.messageTextAlignment = bannerType.map { $0.textAlignment }

    self.bannerMessage = self.messageBannerConfiguration.signal.skipNil().map(second)
+    self.bannerMessageAccessibilityLabel = self.bannerMessage
+      // todo: Needs to be localized - Strings.banner_tap_to_dismiss
+      .map { "\($0), double tap to dismiss" }


Has this been added to our strings yet?

I think it's always a good idea to use the localizedString() function directly if we don't yet have it in Strings.swift that way if we forget about this it's going to use localization or if we need an easy way to find it later we can just search for uses of localizedString().

I completely forgot about this...good catch Justin.

Created https://github.com/kickstarter/kickstarter/pull/14146 and will block this PR until we have those strings.

Also I had to update the string to be in form of a sentence to better match server response..

Before

After

Also note that we should remove project_count_projects_backed when https://github.com/kickstarter/kickstarter/pull/13979#pullrequestreview-198853575 gets merged

justinswart · 2019-01-29T19:54:57Z

Library/ViewModels/MessageBannerViewModel.swift

+    let dismissBanner = self.bannerViewIsHiddenProperty.signal
+      .filter(isFalse)
+
+    // Dismisses the banner after 4 seconds when VoiceOver is OFF


Were we certain about the use of debounce here vs delay? I remember chatting about it but can't recall 😅

Yes, we actually really want to use debounce. The banner was showing a weird behaviour when using delay.

Co-Authored-By: dusi <pavel.dusatko@gmail.com>

…into a11y-settings

dusi · 2019-02-04T18:06:37Z

Addressed all outstanding feedback

dusi · 2019-02-04T18:09:12Z

Kickstarter-iOS/Locales/de.lproj/Localizable.strings

@@ -247,6 +247,7 @@
 "Manage_your_pledge_below" = "Folgenden Beitrag verwalten";
 "Manage_your_reward" = "Belohnung verwalten";
 "Message_backer" = "Nachricht an Unterstützer senden";
+"Message_banner.accessibility.Double_tap_to_dismiss" = "Double tap to dismiss.";


Currently this string is not localized, but based on the conversation we had with @peat it should not delay merging stuff to master/:shipit:

justinswart

Looking good, minor comments!

justinswart · 2019-02-05T23:08:50Z

Kickstarter-iOS/Views/Controllers/SettingsNotificationsViewController.swift

+        }
+
+        self.emailPickerViewTopConstraint.constant = isHidden ? 0 : self.emailFrequencyPickerView.frame.height
+        self.view.setNeedsLayout()


Have you tested without setNeedsLayout()? I think we only need layoutIfNeeded().

Not originally my code...but have tried removing and it looks like everything works. Agree that we should only need to call layoutIfNeeded().

justinswart · 2019-02-05T23:12:19Z

Library/ViewModels/MessageBannerViewModel.swift

@@ -53,14 +54,33 @@ MessageBannerViewModelInputs, MessageBannerViewModelOutputs {
    self.messageTextAlignment = bannerType.map { $0.textAlignment }

    self.bannerMessage = self.messageBannerConfiguration.signal.skipNil().map(second)
+    self.bannerMessageAccessibilityLabel = self.bannerMessage
+      .map { "\($0) \(Strings.Message_banner_accessibility_Double_tap_to_dismiss())" }


I guess we don't have much choice here but just something to bear in mind is that string concatenation can break translations for languages where the direction isn't left to right.

justinswart · 2019-02-05T23:13:04Z

Library/ViewModels/SettingsPrivacyViewModel.swift

+      self.didCancelSocialOptOutProperty.signal,
+      self.didConfirmSocialOptOutProperty.signal
+    )
+      .filter { _ in AppEnvironment.current.isVoiceOverRunning() }


Could we align the . with the last parenthesis?

justinswart

Great work!

dusi added the WIP label Jan 7, 2019

dusi changed the title ~~♿️ Settings~~ [WIP] ♿️ Settings Jan 7, 2019

dusi changed the title ~~[WIP] ♿️ Settings~~ [WIP] ♿️ - Settings Jan 8, 2019

dusi force-pushed the a11y-settings branch from 00772d7 to d6721f4 Compare January 8, 2019 21:59

dusi force-pushed the a11y-settings branch from 90ec7fb to ade427d Compare January 17, 2019 22:28

dusi changed the title ~~[WIP] ♿️ - Settings~~ ♿️ - Settings Jan 29, 2019

dusi added needs review and removed WIP labels Jan 29, 2019

dusi requested review from ifbarrera, cdolm92 and justinswart January 29, 2019 17:21

dusi and others added 11 commits January 29, 2019 09:22

Accessibility - Settings - Dismiss button (#532)

3768fbe

* Add accessibility label to dismiss button & fix the minimum width * Set navigation item properties using key paths

Accessibility - Settings - Rows (#534)

9217fd3

* Add button traits to settings cells * Invert disclosure indicators in Settings > Help

Add accessibility hint in case email is unverified (#535)

d3a8246

Accessibility - Settings - Change email - Current email (#537)

e232781

* Group current email title and value for a better accessibility * Simplify the way we set accessibility label on current email container

Expose delete my kickstarter account button instead of the label (#549)

a82ddec

♿️- Settings - Change email / Change password input fields (#545)

89dc48d

* textfield a11y improvement in change email and password screens * change stackview alignment * new snapshots * fixed strings

♿️ - Settings > Notifications - Manage project notifications + Email …

ac4d24e

…frequency (#552) * Add button traits to project notifications & email frequency rows * Remove unused accessibility hint * Remove accessibility value

dusi force-pushed the a11y-settings branch from d69c736 to ac4d24e Compare January 29, 2019 17:22

justinswart reviewed Jan 29, 2019

View reviewed changes

dusi and others added 6 commits January 30, 2019 18:08

Merge branch 'master' into a11y-settings

9649519

Infer UIView.AnimationOptions type

93d80ca

Combine conditions

5be8d8f

Use more functional return

d9728de

Co-Authored-By: dusi <pavel.dusatko@gmail.com>

Merge branch 'a11y-settings' of https://github.com/kickstarter/ios-oss …

b8133be

…into a11y-settings

Fix Github's messed up suggestion

853567a

Update accessibility label to better match with localized server errors

e303019

dusi added blocked a PR that is blocked for external reasons and removed needs review labels Jan 31, 2019

dusi added 2 commits February 4, 2019 09:54

Add localized string

9d3e92d

Use localized string

59542ff

dusi commented Feb 4, 2019

View reviewed changes

dusi removed the blocked a PR that is blocked for external reasons label Feb 4, 2019

justinswart reviewed Feb 5, 2019

View reviewed changes

dusi added 3 commits February 5, 2019 19:09

Align filter

1ceb7c3

Remove unused call to setNeedsLayout

8b8f21b

Merge branch 'master' into a11y-settings

c10d5ae

justinswart approved these changes Feb 6, 2019

View reviewed changes

dusi merged commit d2cf6f9 into master Feb 6, 2019

dusi deleted the a11y-settings branch February 6, 2019 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♿️ - Settings #546

♿️ - Settings #546

dusi commented Jan 7, 2019 •

edited

Loading

justinswart left a comment

justinswart Jan 29, 2019

dusi Jan 30, 2019

justinswart Jan 29, 2019

dusi Jan 31, 2019

justinswart Jan 29, 2019

justinswart Jan 29, 2019

dusi Jan 31, 2019

dusi Jan 31, 2019

dusi Jan 31, 2019

justinswart Jan 29, 2019

dusi Jan 30, 2019

dusi commented Feb 4, 2019

dusi Feb 4, 2019

justinswart left a comment

justinswart Feb 5, 2019

dusi Feb 6, 2019

justinswart Feb 5, 2019

justinswart Feb 5, 2019

justinswart left a comment

♿️ - Settings #546

♿️ - Settings #546

Conversation

dusi commented Jan 7, 2019 • edited Loading

📲 What

🤔 Why

🛠 How

👀 See

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusi commented Feb 4, 2019

Choose a reason for hiding this comment

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart left a comment

Choose a reason for hiding this comment

dusi commented Jan 7, 2019 •

edited

Loading