feat(a11y): directory search result announcement #6788
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DirectoryView
participant Backend
participant AccessibilityInfo
participant ScreenReader
User->>DirectoryView: Enter search and submit
DirectoryView->>Backend: GET /directory?query...
Backend-->>DirectoryView: PaginatedResult { result: [...], count }
rect rgb(230, 245, 230)
Note over DirectoryView,AccessibilityInfo: Accessibility announcement flow
DirectoryView->>DirectoryView: announceSearchResults(count)
alt count == 0
DirectoryView->>AccessibilityInfo: announce(i18n('No_results_found'))
else count == 1
DirectoryView->>AccessibilityInfo: announce(i18n('One_result_found'))
else count > 1
DirectoryView->>AccessibilityInfo: announce(i18n('Search_Results_found', {count}))
end
AccessibilityInfo->>ScreenReader: Deliver spoken message
ScreenReader->>User: Read out results message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Areas to inspect:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.67.0.107704 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNS4XymYHDhKG1LW9bhakn3Je4uvIDJEDGyzaZePO7qf2NmcBx9AQVaKmwdR7utsIz9DQh2Ty6HnERoquxIA |
|
iOS Build Available Rocket.Chat Experimental 4.67.0.107706 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.67.0.107719 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNR4xQoAZKV50o8KUXYH7z-Dl_Mefab5bZnYnV3kdX_80eYxfRt3aoHIsrAlqEEp_4y6DYZLE0TmtWITo8RG |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/Avatar/Avatar.tsx (1)
98-115: Nested accessible elements whenonPressis provided.When
onPressis provided, both theTouchable(line 100) and the rootView(line 108) haveaccessible={accessible}. This creates nested accessible elements in the accessibility tree, which can confuse screen readers. Typically, only the interactiveTouchableshould be accessible in this case, and the outerViewshould be excluded from the accessibility tree.Apply this diff to prevent nested accessible elements:
if (onPress) { image = ( <Touchable accessible={accessible} accessibilityLabel={avatarAccessibilityLabel} onPress={onPress}> {image} </Touchable> ); } return ( <View - accessible={accessible} + accessible={onPress ? false : accessible} accessibilityLabel={!onPress ? avatarAccessibilityLabel : undefined} style={[avatarStyle, style]} testID='avatar'> {image} {children} </View> );This ensures that when
onPressexists, only theTouchableis accessible, and whenonPressis absent, only theViewis accessible.
🧹 Nitpick comments (3)
app/definitions/rest/v1/directory.ts (1)
4-12: RedundantcountinPaginatedResultpayload; clarify intended shape
PaginatedResult<T>already includes a top‑levelcount: number, so changingTto{ result: IServerRoom[]; count: number }effectively re‑declares the samecountproperty instead of adding a new field. This is redundant and can be misleading for consumers trying to distinguish betweencountandtotal, or between top‑level vs per‑query counts.If the API response really added a new nested structure (e.g.
{ result: { rooms: IServerRoom[]; count: number } }) or a separate result count, consider:
- Either keeping the original type and relying on
count/totalfromPaginatedResult, or- Introducing a differently named field (e.g.
resultsCount) inT, or- Wrapping rooms in an inner object if that matches the actual JSON.
Please confirm the actual response shape and adjust the type accordingly.
app/i18n/locales/ar.json (1)
376-376: Improve Arabic plural search-result phrasing for clarity
One_result_foundis fine, but:"Search_Results_found": "{{count}} تم العثور على نتائج."reads unnaturally. A clearer and more idiomatic phrasing would place the count inside the verb phrase, e.g.:
- "Search_Results_found": "{{count}} تم العثور على نتائج." + "Search_Results_found": "تم العثور على {{count}} نتيجة."This will sound more natural for Arabic users and screen readers.
Also applies to: 486-486
app/i18n/locales/cs.json (1)
582-582: Czech plural nuance: consider improving multi-count phrasing."{{count}} výsledků nalezeno." is acceptable, but Czech ideally distinguishes 2–4 ("výsledky") vs 5+ ("výsledků"). If plural rules aren’t supported, a neutral phrasing like "Nalezeno {{count}} výsledků." is commonly used across counts. If your i18n layer supports ICU plurals, prefer proper pluralization.
Confirm whether your i18n supports ICU plural rules; if yes, we can propose an ICU-based variant for cs.
Also applies to: 734-734
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
app/containers/DirectoryItem/__snapshots__/DirectoryItem.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (31)
app/containers/Avatar/Avatar.tsx(2 hunks)app/containers/Avatar/AvatarContainer.tsx(2 hunks)app/containers/Avatar/interfaces.ts(1 hunks)app/containers/DirectoryItem/index.tsx(1 hunks)app/definitions/rest/v1/directory.ts(1 hunks)app/i18n/locales/ar.json(2 hunks)app/i18n/locales/bn-IN.json(2 hunks)app/i18n/locales/cs.json(2 hunks)app/i18n/locales/de.json(2 hunks)app/i18n/locales/en.json(2 hunks)app/i18n/locales/es.json(2 hunks)app/i18n/locales/fi.json(2 hunks)app/i18n/locales/fr.json(2 hunks)app/i18n/locales/hi-IN.json(2 hunks)app/i18n/locales/hu.json(2 hunks)app/i18n/locales/it.json(2 hunks)app/i18n/locales/ja.json(2 hunks)app/i18n/locales/nl.json(2 hunks)app/i18n/locales/nn.json(2 hunks)app/i18n/locales/no.json(2 hunks)app/i18n/locales/pt-BR.json(2 hunks)app/i18n/locales/pt-PT.json(2 hunks)app/i18n/locales/ru.json(2 hunks)app/i18n/locales/sl-SI.json(2 hunks)app/i18n/locales/sv.json(2 hunks)app/i18n/locales/ta-IN.json(2 hunks)app/i18n/locales/te-IN.json(2 hunks)app/i18n/locales/tr.json(2 hunks)app/i18n/locales/zh-CN.json(2 hunks)app/i18n/locales/zh-TW.json(2 hunks)app/views/DirectoryView/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/Avatar/Avatar.tsx (1)
app/containers/Avatar/Avatar.stories.tsx (1)
Touchable(48-48)
app/definitions/rest/v1/directory.ts (2)
app/definitions/rest/helpers/PaginatedResult.ts (1)
PaginatedResult(1-5)app/definitions/IRoom.ts (1)
IServerRoom(152-236)
🔇 Additional comments (19)
app/i18n/locales/nl.json (1)
470-470: Localization keys correctly added for search result announcements.Both new translation keys are properly formatted, alphabetically positioned, and use correct Dutch grammar. The template variable
{{count}}supports both singular and plural result announcements, which aligns well with the accessibility feature for screen-reader users.Also applies to: 592-592
app/i18n/locales/it.json (1)
410-410: Italian translations look good.Both new i18n entries follow the existing naming conventions, maintain correct alphabetical ordering, use natural Italian grammar, and are properly formatted as JSON strings. The plural/singular distinction (
One_result_foundvs.Search_Results_found) with the{{count}}placeholder correctly supports dynamic result announcements for accessibility.Confirm that these keys are consistently added across all 25 locales mentioned in the PR summary and that they're properly wired into the DirectoryView component for screen reader announcements.
Also applies to: 521-521
app/i18n/locales/hu.json (1)
545-545: Localization keys properly added for search result announcements.Both keys follow the established i18n pattern and Hungarian grammar conventions:
- Line 545: Singular form for announcing a single result
- Line 687: Templated form for announcing multiple results with dynamic count
Positioning is alphabetically correct, and translations align well with the accessibility objectives to announce search result counts to screen readers.
Also applies to: 687-687
app/i18n/locales/sl-SI.json (1)
482-482: Slovene translations for search result announcements look accurate and follow established conventions.The translations are grammatically correct and appropriately use:
- Singular form ("Najden je en rezultat.") for one result
- Plural form with genitive plural ("zadetkov najdenih.") for multiple results
- Proper {{count}} placeholder for dynamic substitution
- Correct alphabetical ordering within the locale file
However, I cannot verify from the provided context whether these keys are actually referenced in the DirectoryView code or whether the i18n system correctly routes between the singular and plural forms.
Please confirm that:
- The DirectoryView component references these keys correctly when announcing search results
- The i18n system (or similar pluralization logic) properly selects between "One_result_found" (when count === 1) and "Search_Results_found" (when count > 1)
You can verify this by checking the DirectoryView implementation and confirming the announcement logic handles both keys appropriately.
Also applies to: 605-605
app/i18n/locales/pt-PT.json (1)
354-354: Ensure translations are appropriate for screen reader announcements.Both translations are grammatically correct Portuguese. However, verify that these phrasing choices are appropriate for accessibility announcements to screen reader users:
- "Um resultado encontrado." (One result found)
- "{{count}} resultados encontrados." ({{count}} results found)
Consider whether these should include additional context like "Search results:" prefix or whether they're announced as standalone statements. Check how other accessibility announcement keys are phrased in the locale file for consistency.
Also applies to: 429-429
app/i18n/locales/es.json (1)
269-269: Spanish result-count strings look correct and consistent
One_result_foundandSearch_Results_foundare grammatically fine and align with how these keys are used (static singular vs{{count}}pluralized form). No changes needed.Also applies to: 343-343
app/i18n/locales/tr.json (1)
393-393: Turkish result-count translations are accurate
One_result_foundandSearch_Results_foundread naturally in Turkish and correctly use{{count}}only in the plural form. Nothing to adjust.Also applies to: 504-504
app/i18n/locales/no.json (1)
572-572: Norwegian result messages look good
Ett resultat funnet.and{{count}} resultater funnet.are idiomatic and match the intended singular/plural split. No further changes needed.Also applies to: 722-722
app/i18n/locales/ru.json (1)
500-500: Russian singular/plural handling is consistent
Один результат найден.for singular andНайдено {{count}} результатов.for plural work well, assuming the UI usesOne_result_foundfor count === 1 andSearch_Results_foundotherwise. Looks good.Also applies to: 625-625
app/i18n/locales/en.json (1)
604-604: English baseline strings are appropriate
One result found.and{{count}} results found.are clear, concise, and suitable for screen-reader announcements. No changes required.Also applies to: 762-762
app/i18n/locales/zh-TW.json (1)
393-393: LGTM: zh-TW translations read naturally.
- Wording, classifier 個, and punctuation look correct.
Please confirm:
- 0 results uses existing No_results_found.
- Count === 1 uses One_result_found; count > 1 uses Search_Results_found.
Also applies to: 503-503
app/i18n/locales/fr.json (1)
470-470: LGTM: French strings are accurate and idiomatic.Nothing blocking.
Please verify these keys exist in en.json and that DirectoryView/announce logic maps 0/1/>1 correctly.
Also applies to: 592-592
app/i18n/locales/bn-IN.json (1)
544-544: LGTM: bn-IN translations look correct.Punctuation and placeholder usage are consistent.
Please confirm 0/1/>1 routing uses No_results_found / One_result_found / Search_Results_found respectively.
Also applies to: 686-686
app/i18n/locales/sv.json (1)
509-509: LGTM: Swedish phrasing is natural and consistent.No issues spotted.
Double-check that en.json contains these keys and that code paths use them for 1 vs >1 results.
Also applies to: 647-647
app/i18n/locales/de.json (1)
533-533: LGTM: German translations are correct and consistent.All good.
Ensure:
- No_results_found is used for 0.
- One_result_found for 1.
- Search_Results_found for >1.
Also applies to: 673-673
app/views/DirectoryView/index.tsx (1)
2-2: LGTM!The
AccessibilityInfoimport is correctly added to support screen reader announcements for search results.app/containers/Avatar/AvatarContainer.tsx (1)
23-25: LGTM! Clean prop forwarding.The new
accessibleprop is correctly destructured and forwarded to the underlyingAvatarcomponent, maintaining consistency with the existing prop-forwarding pattern.Also applies to: 75-75
app/containers/DirectoryItem/index.tsx (1)
58-58: Good accessibility pattern: child Avatar excluded from tree.Setting
accessible={false}on the nestedAvataris correct. This prevents the Avatar from being announced separately, allowing the parentViewto handle accessibility for the entire directory item as a single, cohesive element.app/containers/Avatar/Avatar.tsx (1)
37-38: LGTM! Public API extended with accessible prop.The
accessibleprop with a default value oftrueprovides sensible out-of-the-box behavior while allowing consumers to control accessibility when needed (e.g., when grouping elements under a parent accessible container).
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
Proposed changes
announce the directory search result count when the user stops typing.
Issue(s)
https://rocketchat.atlassian.net/browse/MA-183
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Accessibility
Localization