🎨 Palette: Add dynamic context to list row action accessibility labels#69
🎨 Palette: Add dynamic context to list row action accessibility labels#69NSEvent wants to merge 3 commits into
Conversation
Added dynamic item names to the `.help()` and `.accessibilityLabel()` modifiers for the Edit and Delete buttons in `MacroListView` and `ScriptListView`. This significantly improves the experience for screen reader users and those relying on tooltips, allowing them to know exactly which item an action applies to in a repeated list. Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughEdit and delete icon buttons in ChangesDynamic accessibility labels for list action buttons
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
XboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swift (1)
198-198: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract repeated "Untitled Script" fallback to avoid drift.
The fallback logic
script.name.isEmpty ? "Untitled Script" : script.nameis repeated five times inScriptRow(display Text, edit help, edit accessibilityLabel, delete help, delete accessibilityLabel). Extract a private computed property:private var displayName: String { script.name.isEmpty ? "Untitled Script" : script.name } // Usage: Text(displayName) ... .help("Edit \(displayName)") .accessibilityLabel("Edit \(displayName)") ... .help("Delete \(displayName)") .accessibilityLabel("Delete \(displayName)")This prevents copy drift and reduces churn if the fallback wording changes later.
Also applies to: 225-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@XboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swift` at line 198, The "Untitled Script" fallback is duplicated across ScriptRow, so extract it into a single private computed property like displayName on the ScriptRow view and use that everywhere Text, help, and accessibilityLabel need the script name. Update the existing script.name.isEmpty checks to reference displayName so the fallback wording stays consistent and only changes in one place.
🧹 Nitpick comments (2)
XboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swift (2)
158-159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider guarding against empty
macro.namefor accessibility quality.
SharedMacroRowinterpolatesmacro.namedirectly without a fallback. Ifmacro.namecan be empty, VoiceOver will read"Edit in shared library"with an awkward pause.ScriptListViewuses"Untitled Script"as a fallback for empty names. Consider whetherMacroListViewshould mirror that defensive pattern for consistency.// Example pattern from ScriptListView: .help("Edit \(macro.name.isEmpty ? "Untitled Macro" : macro.name) in shared library")If the data model guarantees non-empty names, this can be skipped; otherwise apply a fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@XboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swift` around lines 158 - 159, The accessibility/help text in SharedMacroRow currently interpolates macro.name directly, which can produce awkward output when the name is empty. Update the help and accessibilityLabel strings in MacroListView to use a fallback title like ScriptListView does for empty names, so the edit action remains readable to VoiceOver even when macro.name is blank.
209-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame empty-name concern for
MacroRowedit and delete buttons.Lines 209-210 and 217-218 interpolate
macro.namewithout fallback. If empty names are possible, VoiceOver produces"Edit "or"Delete "with trailing silence. Align with the defensive pattern used inScriptListViewor document the non-empty invariant.Additionally, verify that
MacroRow's delete button should not include an.accessibilityHintdescribing the destructive action, as macOS VoiceOver users benefit from hints on destructive buttons.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@XboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swift` around lines 209 - 218, The `MacroRow` edit/delete controls in `MacroListView` use `macro.name` directly in `help` and `accessibilityLabel`, so add the same empty-name fallback pattern used elsewhere (or enforce/document a non-empty name invariant) to avoid empty VoiceOver text. Also review `MacroRow`’s delete `Button` to ensure it includes an `.accessibilityHint` that describes the destructive action for macOS VoiceOver users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.Jules/palette.md:
- Around line 25-31: The palette entries are out of date order in the section
containing the 2024-05-19 and 2026-06-23 notes, so update the placement to match
the file’s established ordering convention. Reorder the affected entries in the
palette document so the date sequence is consistent throughout the list, using
the existing entry headings as the anchors to move the content.
---
Outside diff comments:
In
`@XboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swift`:
- Line 198: The "Untitled Script" fallback is duplicated across ScriptRow, so
extract it into a single private computed property like displayName on the
ScriptRow view and use that everywhere Text, help, and accessibilityLabel need
the script name. Update the existing script.name.isEmpty checks to reference
displayName so the fallback wording stays consistent and only changes in one
place.
---
Nitpick comments:
In `@XboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swift`:
- Around line 158-159: The accessibility/help text in SharedMacroRow currently
interpolates macro.name directly, which can produce awkward output when the name
is empty. Update the help and accessibilityLabel strings in MacroListView to use
a fallback title like ScriptListView does for empty names, so the edit action
remains readable to VoiceOver even when macro.name is blank.
- Around line 209-218: The `MacroRow` edit/delete controls in `MacroListView`
use `macro.name` directly in `help` and `accessibilityLabel`, so add the same
empty-name fallback pattern used elsewhere (or enforce/document a non-empty name
invariant) to avoid empty VoiceOver text. Also review `MacroRow`’s delete
`Button` to ensure it includes an `.accessibilityHint` that describes the
destructive action for macOS VoiceOver users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5b85f89-1f44-4f1b-9e35-b0211352193a
📒 Files selected for processing (3)
.Jules/palette.mdXboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swiftXboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swift
| ## 2026-06-23 - [SwiftUI Button Accessibility] | ||
| **Learning:** Found a pattern where SwiftUI icon-only buttons or minimal UI elements were given `.help()` modifiers for hover tooltips but lacked `.accessibilityLabel()` modifiers for screen readers. | ||
| **Action:** When adding `.help()` to buttons, always pair it with a corresponding `.accessibilityLabel()` to ensure full accessibility. | ||
|
|
||
| ## 2024-05-19 - [Missing Dynamic Context in Repeated List Action Buttons] | ||
| **Learning:** Icon-only action buttons (like "Edit" or "Delete") inside `ForEach` lists or repeating views often use static strings for `.help()` and `.accessibilityLabel()`, such as `.help("Edit")`. This causes a poor experience for screen reader users and those relying on tooltips, as they encounter multiple identical "Edit" labels without knowing *what* item is being edited. | ||
| **Action:** When adding `.help()` and `.accessibilityLabel()` to action buttons within a list or loop, always utilize dynamic context from the current item (e.g., `.help("Edit \(item.name)")`) to disambiguate the action for all users. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix chronological ordering of palette entries.
The new 2024-05-19 entry is placed between 2024-06-03 and 2026-06-23, breaking consistent date ordering. If the file is reverse-chronological (newest first), 2024-05-19 should appear after 2026-06-23 at the bottom. If chronological (oldest first), it should appear before 2024-06-03 at the top. Either move it to maintain the established ordering convention, or add a note explaining the ordering scheme.
Current order: 2024-06-03 → 2024-05-19 → 2026-06-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.Jules/palette.md around lines 25 - 31, The palette entries are out of date
order in the section containing the 2024-05-19 and 2026-06-23 notes, so update
the placement to match the file’s established ordering convention. Reorder the
affected entries in the palette document so the date sequence is consistent
throughout the list, using the existing entry headings as the anchors to move
the content.
Added dynamic item names to the `.help()` and `.accessibilityLabel()` modifiers for the Edit and Delete buttons in `MacroListView` and `ScriptListView`. This significantly improves the experience for screen reader users and those relying on tooltips, allowing them to know exactly which item an action applies to in a repeated list. Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
Added dynamic item names to the `.help()` and `.accessibilityLabel()` modifiers for the Edit and Delete buttons in `MacroListView` and `ScriptListView`. This significantly improves the experience for screen reader users and those relying on tooltips, allowing them to know exactly which item an action applies to in a repeated list. Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
1 participant
💡 What: Updated static "Edit" and "Delete" string literals in
.help()and.accessibilityLabel()modifiers withinMacroListViewandScriptListViewto include dynamic context (e.g.macro.name).🎯 Why: Without dynamic context, VoiceOver users hear multiple identical "Edit, Button" announcements in list views, making it impossible to know which row's action they are currently focused on without navigating backward.
📸 Before/After: (Visual changes are on hover tooltips, screen reader changes are non-visual).
♿ Accessibility: Vastly improves context and disambiguation for screen reader navigation in list views.
PR created automatically by Jules for task 174939266289422751 started by @NSEvent
Summary by CodeRabbit