Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

offline notice: Queue non-urgent a11y announcements behind in-progress #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

supra08
Copy link

@supra08 supra08 commented May 9, 2024

ones
fixes: zulip#5611

Copy link

dashwave bot commented May 9, 2024

Build Status ✅ Successful
Build Time 4m55s
📱 Emulation Link https://console.dashwave.io/emulator/1732/preview?device=emulator
Logs See Logs

@supra08
Copy link
Author

supra08 commented Oct 1, 2024

@dashwave /fix

@supra08
Copy link
Author

supra08 commented Oct 1, 2024

@dashwave /review

Copy link

dashwave bot commented Oct 1, 2024

Pull Request Description

Title: Queue non-urgent a11y announcements behind in-progress ones

What is this?

This pull request addresses the need to improve the accessibility features within the Zulip application by ensuring that non-urgent announcements are queued behind in-progress ones. This improves the user experience for screen reader users, ensuring that important updates aren't missed due to overlapping announcements. The motivation for this change was the raised issue zulip#5611, which identified the need for such functionality to comply with modern accessibility standards.

Changes

Code Changes:

  1. In src/boot/OfflineNoticeProvider.js:
    • Function OfflineNoticeProvider(props: ProviderProps): Node:
      • Modified the announcements for shouldShowUncertaintyNotice and instances where Zulip is online to use announceForAccessibilityWithOptions with { queue: true }, ensuring that these announcements are queued behind any in-progress ones.
      • Removed the TODO comments that referenced react-native-68 as it is now handled properly with announceForAccessibilityWithOptions.

Documentation Updates:

  1. In src/boot/OfflineNoticeProvider.js:
    • Updated in-line comments to reflect the usage of announceForAccessibilityWithOptions.

Pull Request Review

Code Quality and Readability

  • The code changes improve the readability by replacing the announceForAccessibility with announceForAccessibilityWithOptions with the appropriate options.
  • The removal of the TODO comments cleaned up the code, removing obsolete notes.
  • The new comments explain the purpose behind the change, which adds to the maintainability of the code.

Integration with Existing Codebase

  • The modifications integrate seamlessly with the existing codebase, utilizing existing React Native accessibility functions.
  • The change preserves the current logical flow; as such, it does not introduce any unexpected behavior into the application.

Documentation and Comments

  • The comments within the code have been kept concise and informative, explaining why the new functionality has been implemented.
  • Removing the TODOs eliminates any potential confusion about pending work regarding react-native-68.

Testing and Functionality Verification

  • Ensure to run existing tests related to announcements and add new ones if necessary to validate queuing functionality.
  • Verify through a suite of accessibility tests that this change improves the user experience for screen reader users, ensuring that they receive notifications in a sequential and non-overlapping manner.

In summary, the provided modifications enhance the accessibility announcements by queuing non-urgent announcements properly, resolving issue zulip#5611. The changes are well-documented, clean, and integrate well into the existing codebase. Further testing and validation will confirm the improvements and ensure the stability of the application.

@supra08
Copy link
Author

supra08 commented Oct 4, 2024

@dashwave /review

Copy link

dashwave bot commented Oct 4, 2024

Pull Request

Title

Queue Non-Urgent Accessibility Announcements Behind In-Progress Ones in OfflineNoticeProvider

What is this?

This pull request aims to enhance the accessibility announcement handling within the OfflineNoticeProvider component. The component now uses React Native's capability to queue accessibility announcements, which ensures that non-urgent messages do not interrupt ongoing announcements. This update is especially beneficial in providing seamless auditory feedback to users, improving overall user experience for visually impaired individuals who rely on screen readers.

This change addresses issue zulip#5611 by ensuring that users receive structured and non-disruptive auditory notifications regarding the application's online and offline status.

Changes

Code Changes:

  1. In src/boot/OfflineNoticeProvider.js:
    • Function OfflineNoticeProvider:
      • AccessibilityInfo.announceForAccessibility was replaced with AccessibilityInfo.announceForAccessibilityWithOptions in two places. The method now opts to queue accessibility announcements by passing { queue: true } as options.
      • This modification affects the way the messages "Zulip’s Internet connection is uncertain." and "Zulip is online." are delivered, ensuring they do not interrupt other ongoing announcements.

Documentation Updates:

  • No changes in external documentation are necessary.

Pull Request Review

Code Review

General Feedback:

The implementation ensures accessibility announcements are queued correctly, thereby offering a non-intrusive user experience. This aligns well with accessibility best practices, ensuring users are not overwhelmed by immediate auditory feedback when other messages are in progress.

Code Quality:

  • The code changes propose clear and minimalistic adjustments while effectively leveraging the AccessibilityInfo API enhancements.
  • Usage of announceForAccessibilityWithOptions is concise and adheres to React Native's recommended practices for accessibility improvements.

Code Integration:

  • The changes integrate smoothly with the existing OfflineNoticeProvider component. The replacement of methods is straightforward and does not introduce any breaking changes.
  • Local state variables such as haveAnnouncedUncertain and haveAnnouncedOffline remain effective in ensuring announcements do not repeat unnecessarily.

Documentation:

  • While the inline comments sufficiently address the purpose of changes, further comments could improve clarity for new contributors. Specifically, a brief note explaining why queuing is advantageous in this context could be added.

Recommendations:

  • Consider running thorough tests to confirm the behavior of queued announcements in various scenarios since auditory feedback timing is crucial.

The execution of this enhancement is well-aligned with the intended goals of ensuring a balanced auditory notification system while maintaining compliance with accessibility standards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set queue on accessibility announcements in OfflineNoticeProvider
2 participants