🎨 Palette: Add contextual disambiguation to list action tooltips and accessibility labels#77
Open
NSEvent wants to merge 4 commits into
Open
Conversation
…d tooltips Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
Contributor
|
👋 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. |
Contributor
📝 WalkthroughWalkthroughThis PR updates ChangesContextual Accessibility Labels
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
…ation, add dynamic context to list action accessibility labels and tooltips Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
…accessibility labels 💡 **What:** The UX enhancement added dynamic context from the current list item to `.help()` tooltips and `.accessibilityLabel()` attributes for icon-only list actions in Macros, Scripts, and Gestures. 🎯 **Why:** Previously, VoiceOver users and hover tooltips would simply read "Edit" or "Delete" multiple times in a row for repeated list items. ♿ **Accessibility:** Added significant disambiguation to screen reader navigation. Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
…accessibility labels 💡 **What:** The UX enhancement added dynamic context from the current list item to `.help()` tooltips and `.accessibilityLabel()` attributes for icon-only list actions in Macros, Scripts, and Gestures. 🎯 **Why:** Previously, VoiceOver users and hover tooltips would simply read "Edit" or "Delete" multiple times in a row for repeated list items. ♿ **Accessibility:** Added significant disambiguation to screen reader navigation. Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
XboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swift (1)
158-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated fallback-name logic to reduce duplication.
The
macro.name.isEmpty ? "Unnamed Macro" : macro.nameternary is duplicated 4 times acrossSharedMacroRowandMacroRow(once per.help/.accessibilityLabelpair, per button). Extracting it into a computed property would avoid the two strings drifting if only one is edited later.♻️ Proposed refactor
struct SharedMacroRow: View { let macro: TriggerKitCore.AutomationMacro var onEdit: () -> Void + private var displayName: String { + macro.name.isEmpty ? "Unnamed Macro" : macro.name + } + var body: some View { ... .buttonStyle(.borderless) - .help("Edit \(macro.name.isEmpty ? "Unnamed Macro" : macro.name) in shared library") - .accessibilityLabel("Edit \(macro.name.isEmpty ? "Unnamed Macro" : macro.name) in shared library") + .help("Edit \(displayName) in shared library") + .accessibilityLabel("Edit \(displayName) in shared library")struct MacroRow: View { let macro: Macro var onEdit: () -> Void var onDelete: () -> Void + private var displayName: String { + macro.name.isEmpty ? "Unnamed Macro" : macro.name + } + var body: some View { ... .buttonStyle(.borderless) - .help("Edit \(macro.name.isEmpty ? "Unnamed Macro" : macro.name)") - .accessibilityLabel("Edit \(macro.name.isEmpty ? "Unnamed Macro" : macro.name)") + .help("Edit \(displayName)") + .accessibilityLabel("Edit \(displayName)") Button(action: onDelete) { Image(systemName: "trash") .foregroundColor(.red.opacity(0.8)) } .buttonStyle(.borderless) - .help("Delete \(macro.name.isEmpty ? "Unnamed Macro" : macro.name)") - .accessibilityLabel("Delete \(macro.name.isEmpty ? "Unnamed Macro" : macro.name)") + .help("Delete \(displayName)") + .accessibilityLabel("Delete \(displayName)")Also applies to: 209-218
🤖 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 fallback macro-name ternary is duplicated in SharedMacroRow and MacroRow, so extract it into a shared computed property or helper on the relevant row view and reuse it for both .help and .accessibilityLabel. Update the edit button labels to reference that single source of truth so the “Unnamed Macro” fallback stays consistent across the shared library and regular macro rows.XboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swift (1)
225-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated fallback-name logic to reduce duplication.
Same duplication pattern as in
MacroListView.swift:script.name.isEmpty ? "Untitled Script" : script.namerepeats 4 times (also duplicated with line 198'sTextusage). Consider a computed property.♻️ Proposed refactor
struct ScriptRow: View { let script: Script var onEdit: () -> Void var onDelete: () -> Void + private var displayName: String { + script.name.isEmpty ? "Untitled Script" : script.name + } + var body: some View { ... - Text(script.name.isEmpty ? "Untitled Script" : script.name) + Text(displayName) ... .buttonStyle(.borderless) - .help("Edit \(script.name.isEmpty ? "Untitled Script" : script.name)") - .accessibilityLabel("Edit \(script.name.isEmpty ? "Untitled Script" : script.name)") + .help("Edit \(displayName)") + .accessibilityLabel("Edit \(displayName)") Button(action: onDelete) { Image(systemName: "trash") .foregroundColor(.red.opacity(0.8)) } .buttonStyle(.borderless) - .help("Delete \(script.name.isEmpty ? "Untitled Script" : script.name)") - .accessibilityLabel("Delete \(script.name.isEmpty ? "Untitled Script" : script.name)") + .help("Delete \(displayName)") + .accessibilityLabel("Delete \(displayName)")🤖 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` around lines 225 - 234, The fallback-name logic for scripts is duplicated across the action labels in ScriptListView, so centralize it behind a single computed property or helper on the view/model and reuse it for the Edit/Delete help and accessibility labels, matching the pattern already used in MacroListView. Update the Button-related strings in ScriptListView to reference that shared script display name instead of repeating script.name.isEmpty ? "Untitled Script" : script.name multiple times, and keep the same behavior for the Text usage nearby.
🤖 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.
Nitpick comments:
In `@XboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swift`:
- Around line 158-159: The fallback macro-name ternary is duplicated in
SharedMacroRow and MacroRow, so extract it into a shared computed property or
helper on the relevant row view and reuse it for both .help and
.accessibilityLabel. Update the edit button labels to reference that single
source of truth so the “Unnamed Macro” fallback stays consistent across the
shared library and regular macro rows.
In
`@XboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swift`:
- Around line 225-234: The fallback-name logic for scripts is duplicated across
the action labels in ScriptListView, so centralize it behind a single computed
property or helper on the view/model and reuse it for the Edit/Delete help and
accessibility labels, matching the pattern already used in MacroListView. Update
the Button-related strings in ScriptListView to reference that shared script
display name instead of repeating script.name.isEmpty ? "Untitled Script" :
script.name multiple times, and keep the same behavior for the Text usage
nearby.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11ad7329-4486-468d-92ce-1d000ba80a9b
📒 Files selected for processing (4)
.Jules/palette.mdXboxControllerMapper/XboxControllerMapper/Views/Macros/MacroListView.swiftXboxControllerMapper/XboxControllerMapper/Views/MainWindow/GestureListViews.swiftXboxControllerMapper/XboxControllerMapper/Views/Scripts/ScriptListView.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 What: The UX enhancement added dynamic context from the current list item (e.g.
macro.name) to.help()tooltips and.accessibilityLabel()attributes for icon-only list actions in Macros, Scripts, Gestures, and Chords.🎯 Why: Previously, VoiceOver users and hover tooltips would simply read "Edit" or "Delete" multiple times in a row for repeated list items, making it difficult to disambiguate which item was currently focused. Using dynamic fallback-aware names resolves this ambiguity.
♿ Accessibility: Added significant disambiguation to screen reader navigation by ensuring context-aware labeling across all action buttons in the main application lists.
PR created automatically by Jules for task 7251807470675885245 started by @NSEvent
Summary by CodeRabbit