Skip to content

Conversation

@Ahmed-Naguib93
Copy link
Contributor

@Ahmed-Naguib93 Ahmed-Naguib93 commented Dec 3, 2025

What is new?

Added a11y for
1- Course details.
2- My progress.
3- Scores.
4- Module item sequence.
5- Assignment comments.
6- Assignment attempts.
7- Assignment
refs: CLX-947, CLX-950, CLX-2896, CLX-946
builds: Student
affects: Student
release note: none

test plan: none

refs: CLX-947
builds: Student
affects: Student
release note: none

test plan: none
@Ahmed-Naguib93 Ahmed-Naguib93 self-assigned this Dec 3, 2025
@Ahmed-Naguib93 Ahmed-Naguib93 added the career Canvas Career Experience label Dec 3, 2025
@claude
Copy link

claude bot commented Dec 3, 2025

Claude Code Review

Updated: 2025-12-03

Critical Issues

🚨 String format error in ProgramSwitcherMapper.swift line 125

  • Typo in format string: "Part of @% program" should be "Part of %@ program"
  • Missing argument specifier will produce malformed output

🚨 Incomplete localization in Localizable.xcstrings

  • "Tap to open assignment details." entry is empty with no localizations defined
  • Code references this string but it won't have translations available

Observations

✓ Good accessibility focus management with proper state tracking
✓ Changed fatalError() to fallback .dueDate (ScoreDetails.swift:43)
✓ Comprehensive accessibility labels implemented

⚠️ 2 critical issues found

@Ahmed-Naguib93 Ahmed-Naguib93 reopened this Dec 3, 2025
@Ahmed-Naguib93 Ahmed-Naguib93 changed the title Feature/career accessibility course details clx 947 [CLX-947, CLX-950][S] Add accessibility course details Dec 3, 2025
@Ahmed-Naguib93 Ahmed-Naguib93 changed the title [CLX-947, CLX-950][S] Add accessibility course details [CLX-947,CLX-950][S] Add accessibility course details Dec 3, 2025
@Ahmed-Naguib93 Ahmed-Naguib93 changed the title [CLX-947,CLX-950][S] Add accessibility course details [CLX-947,CLX-950][S] Add accessibility for course details Dec 3, 2025
@inst-danger
Copy link
Contributor

inst-danger commented Dec 3, 2025

Warnings
⚠️ This pull request will not generate a release note.
⚠️ One or more files are below the minimum test coverage 50%

Affected Apps: Student

Builds: Student

Coverage New % Master % Delta
Canvas iOS 91% 81.06% 9.94%
Core/Core/Common/Extensions/UIKit/UIScreenExtensions.swift 0% -- --
Horizon/Horizon/Sources/Common/View/CourseDetailsAccessibility.swift 0% -- --
Student/Student/WidgetRouting/WidgetRouter+GradeListWidget.swift 0% 0% 0%
Student/Student/Assignments/AssignmentDetails/SubAssignmentsCard/ViewModel/StudentSubAssignmentsCardItem.swift 0% 100% -100%

Generated by 🚫 dangerJS against a1bf26b

@inst-danger
Copy link
Contributor

inst-danger commented Dec 3, 2025

Builds

Commit: address review comments (a1bf26b)
Build Number: 969
Built At: Dec 09 21:20 CET (12/09 01:20 PM MST)

Student

@claude
Copy link

claude bot commented Dec 3, 2025

Claude Code Review

Updated: 2025-12-03

Issues Found:

  • Incomplete localization string (Localizable.xcstrings): 'Tap to open assignment details.' entry has empty body - missing comment and localization. Runtime error when accessed.

  • Inconsistent trailing whitespace in accessibility labels (CourseDetailsAccessibility.swift, HCourse.swift, ProgramSwitcherHeaderView.swift): Some labels end with trailing space, others don't - causes inconsistent VoiceOver descriptions. Should standardize.

  • Missing translator comments (Localizable.xcstrings): 'read' and 'unread' strings lack comments - poor translation guidance for other languages.

  • Focus management issue (CourseProgressView): focusedItemID may not restore properly on navigation back - accessibility usability gap.

  • String interpolation in localized string (CourseDetailsAccessibility.swift:56): Using string interpolation instead of String(format:) breaks translations in languages with different word order.

✅ (issues found)

@claude
Copy link

claude bot commented Dec 4, 2025

Claude Code Review - 2025-12-04

Critical Issues

  • String format type mismatch (SubmissionComment.swift:118): Format string expects Int but receives String. Line 118 uses "Attempt %d" with attemptString parameter—should be "Attempt number %@" or pass the Int attempt value.

  • Accessibility logic error (CourseDetailsAccessibility.swift:102-106): When isCollapsed=true, displays "Double tap to Expanded" (should be opposite). The condition logic is inverted.

  • Format specifier inconsistency (ScoresAssignment.swift:137): "%d comments, %@" format string with arguments array may fail at runtime. Use proper String(format:) with correct type matching.

📋 No crashes, breaking changes, or performance issues detected beyond format string corrections needed.

✅ Issues found

@claude
Copy link

claude bot commented Dec 4, 2025

Claude Code Review

Updated: 2025-12-04

Issues Found

  1. Missing localization string: 'Result: %@' used in ScoresAssignment.accessibilityLabel but not defined in Localizable.xcstrings. Will cause runtime crash.

  2. Breaking API change: ModuleItemListView init now requires focusedID parameter. All call sites updated, but external usage would break.

Approved Items

  • Accessibility labels properly implemented across courses, scores, submissions
  • Async focus restoration uses proper 0.1s DispatchQueue.main.asyncAfter delays
  • Fixed fatalError() in ScoreDetails.SortOption init with .dueDate default
  • Disabled courses properly marked with .disabled(!course.isEnrolled)
  • Consistent accessibility traits with children: .ignore pattern

❌ Issues found

@claude
Copy link

claude bot commented Dec 8, 2025

Claude Code Review

Updated: 2025-12-08

Critical Issues Found

  • Undefined variable in AssignmentSubmissionView: Line uses selectedTe which is defined as a computed property but the property name appears truncated (likely selectedSubmissionType). Verify property name matches usage.

  • Focus state logic issue in ScoresAssignmentsView: The assignmentFocusedID is set via .onChange() and .onAppear() with async delay, but focus is also bound with .accessibilityFocused($assignmentFocusedID). This async/sync mismatch may cause focus to not properly restore on list refresh.

  • Missing nil-coalescing safety: In AssignmentSubmissionView, accessing viewModel.selectedSubmission.title without optional chaining if selectedSubmission could be nil.

Minor Issues

  • Localization strings use verbose format patterns - consider consolidating repeated format strings
  • @AccessibilityFocusState with Bool? could be simplified to Bool

❌ Issues found

@claude
Copy link

claude bot commented Dec 8, 2025

Claude Code Review

Updated: 2025-12-08

Critical Issues

  • Undefined localization string (Localizable.xcstrings): 'Tap to open assignment details.' entry empty with no localizations. ScoresAssignment.swift:146 will crash at runtime.

  • Format string argument type (ScoresAssignment.swift:137-139): '%d comments, %@' format with proper arguments [Int, String] but should verify NSNumber handling for %d.

  • Missing translator context (Localizable.xcstrings): 'Result: %@' string lacks comment guidance for localization.

Approved

✓ AssignmentSubmissionView properly implements selectedSubmissionType
✓ ProgramSwitcherMapper format string correct
✓ Vertical whitespace fixed in IntroBlock
✓ Focus management with proper async delays
✓ Comprehensive accessibility across all features
✓ No breaking API changes or performance issues

❌ (issues found)

@claude
Copy link

claude bot commented Dec 9, 2025

Claude Code Review

Updated: 2025-12-09

Critical Issues Found

  • Empty localization string (Localizable.xcstrings): "Tap to open assignment details." entry is empty with no value. Referenced in ScoresAssignment.swift:146 - will display blank text to accessibility users.

  • String interpolation in localized string (CourseDetailsAccessibility.swift:57): String(localized: "Duration: \(duration)") embeds value in localized string, breaking translations. Should use String(format:) with proper format specifiers.

  • Missing localization values (Localizable.xcstrings): "Double tap to Collapsed" and "Double tap to Expanded" have no stringUnit values - will fall back to English keys.

  • Inconsistent bundle specification (ScoresAssignment.swift:120-146): Some strings specify bundle: .horizon, others don't - may load from wrong bundle at runtime.

  • Focus restoration gap (ExpandingModuleView.swift:59-65): Focus only restored if lastFocusedID exists. First expansion won't set focus, breaking accessibility for VoiceOver users.

✅ (issues found)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

career Canvas Career Experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants