Completion of Mobility section separation in the Callouts section of Settings#201
Completion of Mobility section separation in the Callouts section of Settings#201MBtheOtaku wants to merge 8 commits into
Conversation
steinbro
left a comment
steinbro
left a comment
There was a problem hiding this comment.
Thanks! There are still a few things to iron out before we can merge this; see my comments below.
| // Section Descriptions | ||
| private static let sectionDescriptions: [Section: String] = [ | ||
| .general: "General settings for the app.", | ||
| .audio: "Control how audio interacts with other media.", | ||
| .callouts: "Manage the callouts that help navigate.", | ||
| .streetPreview: "Settings for including unnamed roads.", | ||
| .troubleshooting: "Options for troubleshooting the app.", | ||
| .about: "Information about the app." | ||
| // .telemetry: "Manage data collection and privacy." | ||
| ] |
There was a problem hiding this comment.
These need to be defined as localizable strings rather than hard-coded, so that they will appear in the user's preferred language.
| guard let sectionType = Section(rawValue: section) else { return nil } | ||
|
|
||
| if expandedSections.contains(section) { | ||
| return SettingsViewController.sectionDescriptions[sectionType] |
There was a problem hiding this comment.
I would delete the referenced sectionDescriptions table and make the calls to GDALocalizedString here, like the next few lines do.
| @objc private func handleHeaderTap(_ gesture: UITapGestureRecognizer) { | ||
| guard let header = gesture.view as? UITableViewHeaderFooterView else { return } | ||
| let section = header.tag | ||
|
|
||
| if expandedSections.contains(section) { | ||
| expandedSections.remove(section) | ||
| } else { | ||
| expandedSections.insert(section) | ||
| } | ||
| tableView.reloadSections(IndexSet(integer: section), with: .automatic) | ||
| } |
There was a problem hiding this comment.
I'm not conceptually opposed to making the settings sections collapsible, but we need to take care that the interaction still makes sense for screen reader users. My inclination would be to leave it as a flat list at least for the purposes of this PR. Alternatively, @RDMurray could test to confirm the panel still behaves as expected with VoiceOver.
|
I think you should remove the periods at the end of the callout descriptions in the localized strings. Soundscape will automatically put a comma after the hint so the period may clutter the punctuation. This will make it much easier for Voiceover users to read. The format template should be "Callout Type, Description, state". For example, the Safety callout with the period will look like this: "Safety, Potential hazards like steps and construction areas., on". If we remove the period it looks like this: "Safety, Potential hazards like steps and construction areas, on". Just follow the format of the existing callout descriptions, and you'll be fine. |
|
Thanks for the updates, but I wonder why your new commits didn't pass all the checks. It says 0/1 checks OK. |
|
The tests are still in progress. |
|
I've already translated all the callouts, hints, and footer descriptions into Spanish so I should be ready with Weblate after this gets merged. But if there's anything wrong with the text that you think should be changed, let's do so before merging. I'm curious, are these footer descriptions for sighted users to understand what the features do, and if so, does that mean that I won't be able to read them with Voiceover? Thanks. |
|
They’re primarily just there as extra guidance for sighted users, but I believe VoiceOver will still announce them. |
|
Thanks, so if Voiceover can read the footers, does that mean that we have to turn on the Voiceover hints? |
|
I don't believe so. Hopefully VoiceOver will naturally read them as you look into section's footer. I think you would only need to add an accessibility hint if you want that text to be announced as a hint on a particular control rather than as its own element. |
|
Sorry I took a while to get round to looking at this. I don't see the footers appearing on device. Is it possible they only show on the storyboard canvas? I'll attach a screenshot. 
Given that they don't show, and don't actually add anything I think they should be removed. It seems that the storyboard still has things relating to older versions of the app which are overridden in the ViewController such as the audio section. Lets keep this |
|
Given that M hasn't separated the footers from this pull request yet, is there a way to get it separated out, like open another pull request with the exact same code minus the footers so that it could be merged? Then I could get to work on the Spanish. |
4 participants
Changes should be good hopefully. On my local testing the sections look correct in Settings storyboard