♿️(frontend) align search modal field label with placeholder#2384
♿️(frontend) align search modal field label with placeholder#2384Ovgodd wants to merge 2 commits into
Conversation
6160724 to
865afc8
Compare
|
Size Change: 0 B Total Size: 4.33 MB 📦 View Changed
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR aligns search input field labels with their placeholder text across document search and move modals. QuickSearchInput no longer sets an internal aria-label and drops its Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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)
src/frontend/apps/e2e/__tests__/app-impress/doc-search.spec.ts (1)
39-39: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider updating for consistency with other tests.
Line 39 uses
getByPlaceholderwhile the other tests in this file (lines 110, 123, 171, 173, 195) were updated to usegetByRole('combobox', { name: 'Type the name of a document' }). UsinggetByRolewith the accessible name is the recommended approach for accessibility testing and aligns with the PR's objective to update test selectors to match the new combobox accessible name.♻️ Proposed refactor
- const inputSearch = page.getByPlaceholder('Type the name of a document'); + const inputSearch = page.getByRole('combobox', { name: 'Type the name of a document' });🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-search.spec.ts` at line 39, Replace the use of getByPlaceholder for the document search input with the accessible-role selector to match other tests: change the code that sets inputSearch (currently using page.getByPlaceholder) to use page.getByRole('combobox', { name: 'Type the name of a document' }) so the test queries the combobox by its accessible name; update any references to the inputSearch variable if needed to reflect the new selector usage.
🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-search.spec.ts`:
- Around line 171-174: Extract the repeated combobox selector into a local
variable to avoid duplication: create a constant (e.g., docCombobox) assigned to
page.getByRole('combobox', { name: 'Type the name of a document' }) and then
call docCombobox.click() and docCombobox.fill('sub page search'); update uses in
the test (the code surrounding the existing await page.getByRole(...) calls) to
reference that variable.
---
Outside diff comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-search.spec.ts`:
- Line 39: Replace the use of getByPlaceholder for the document search input
with the accessible-role selector to match other tests: change the code that
sets inputSearch (currently using page.getByPlaceholder) to use
page.getByRole('combobox', { name: 'Type the name of a document' }) so the test
queries the combobox by its accessible name; update any references to the
inputSearch variable if needed to reflect the new selector usage.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 436f1646-45a4-4c82-a71b-83c7384fd146
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/doc-search.spec.tssrc/frontend/apps/impress/src/components/quick-search/QuickSearch.tsxsrc/frontend/apps/impress/src/components/quick-search/QuickSearchInput.tsxsrc/frontend/apps/impress/src/features/docs/doc-search/components/DocSearchModal.tsx
💤 Files with no reviewable changes (1)
- src/frontend/apps/impress/src/components/quick-search/QuickSearchInput.tsx
e3ccc4b to
ee5703e
Compare
Remove redundant aria-label on QuickSearch input, align DocSearchModal labels.
ab3e2df to
6a39f7f
Compare
Purpose
The search modal input exposed three inconsistent names (
aria-label,<label>, andplaceholder). Screen reader users heard a different label than the visible placeholderProposal
aria-labelonQuickSearchInputso the accessible name comes from cmdk's<label>(aria-labelledby) instead of overriding itDocSearchModallabelandplaceholderonType the name of a documentlistLabelonQuickSearchso the results listbox keepsSearch resultsasaria-labelwhen the input label changesdoc-search.spec.tsto match the new combobox accessible name