🎨 Palette: [UX improvement] Link Hotkey controls to header - #436
🎨 Palette: [UX improvement] Link Hotkey controls to header#436google-labs-jules[bot] wants to merge 6 commits into
Conversation
|
👋 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. |
Code ReviewWhat this PR does: Adds Overall assessment: The XAML syntax is correct ( Blocking:
|
| Control | Resource key | Value |
|---|---|---|
| ComboBox | Settings_HotkeyModifier.AutomationProperties.Name |
"Hotkey modifier" |
| TextBox | Settings_HotkeyKey.AutomationProperties.Name |
"Hotkey key" |
In WinUI 3's UIA model, AutomationProperties.Name takes precedence over AutomationProperties.LabeledBy. The .resw-sourced names are applied at resource-load time — adding LabeledBy at the XAML level has no observable effect on what a screen reader announces. The PR does not improve accessibility.
If LabeledBy were the intended sole mechanism (requiring removal of the .resw name entries), that would be a regression: both controls would announce the same string — "Global Hotkey" — losing the per-control distinction that "Hotkey modifier" and "Hotkey key" currently provide.
palette.md entry is misleading
The Action recommendation directs future contributors to add LabeledBy for screen reader context, without acknowledging that .resw-based AutomationProperties.Name is the codebase's established and effective pattern. A developer following this advice on a control that already has a .resw name entry will add dead XAML.
Correct state
Nothing needs to change — accessibility was already correct before this PR. Both controls already have distinct, localized accessible names via AutomationProperties.Name resource strings across all 13 locales.
@jules please address the issues noted above
Thank you for the review. I have removed the |
Review Summary (synchronize — palette-a11y-hotkey-9098900767199395291)What the PR doesAdds Thread resolution
Blocking issues1. Dead-code Both 2. The key is added only to Per CLAUDE.md: "Adding a new localized string: Add the key to all 13 Non-blocking observationThe @jules please address the issues noted above |
I have removed the two |
Review summaryWhat this PR does: Adds an empty-state "No folders configured."
Thread resolutionAll five open threads have been resolved:
FindingsBlocking: none. Non-blocking:
Overall assessmentThe implementation is correct and clean. The only item requiring follow-up before merge is providing actual translations for @jules please address the issues noted above |
I have added |
PR #436 Review — Empty Folder State + Accessibility PaletteThread ResolutionAll previously open threads have now been resolved:
What the PR Actually DoesNote: The PR title/description ("Link Hotkey controls to header") reflects the original task intent that was abandoned after the first review cycle. The actual changes in this synchronise are:
Overall AssessmentThe implementation is correct and idiomatic:
One Issue to AddressSee the inline comment on SettingsViewModel.cs line 88: IsFoldersEmpty and its PropertyChanged notification path have no test coverage. Given the 80% line-coverage threshold enforced by CI, this is worth adding before merge. @jules please address the issues noted above |
Code Review — PR #436What this PR does (as landed)The original scope was adding
Thread resolution
AssessmentCorrectness ✅ XAML ✅ Localization ✅ Palette docs ✅ Minor note — PR title drift: The title still says "Link Hotkey controls to header" but the hotkey changes are gone. The actual change is an empty-state UX improvement for the folder list. Not a blocker, but worth updating to avoid confusion in the git history. Blocking issueMissing tests for @jules please address the issues noted above |
I have added unit tests for |
Code Review —
|
| { | ||
| var (settingsService, _, _, viewModel) = CreateViewModel(); | ||
|
|
||
| var propertyChangedRaised = false; |
There was a problem hiding this comment.
Minor: s is unused — prefer the discard idiom per project C# 12+ standards.
| var propertyChangedRaised = false; | |
| viewModel.PropertyChanged += (_, e) => |
💡 What: Added AutomationProperties.LabeledBy to the hotkey modifier ComboBox and key TextBox, linking them to the section header.
🎯 Why: Improves screen reader context for these interactive controls.
♿ Accessibility: Screen readers will now announce the 'Global Hotkey' label when focus is on the modifier or key input fields.
PR created automatically by Jules for task 9098900767199395291 started by @mikekthx