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

Rework announcements to not use global SHARED object #389

Merged
merged 3 commits into from
Sep 7, 2021

Conversation

pcardune
Copy link
Collaborator

@pcardune pcardune commented Sep 3, 2021

Changes:

  • SHARED.announcer is gone, as is store.queuedAnnouncements and store.muteAnnouncements
  • added mountAnnouncer() function that encapsulates initialization of dom nodes and announcer state
  • added private Announcer class that encapsulates state/semantics around the announcement queue
  • got rid of muteAnnouncements altogether since it didn't appear to ever be set to anything but false

We're still relying on the correct order of operations to be used in that mountAnnouncer() must be called before say() will do anything, but at least now it's documented. I'm not sure why the dom node that holds the announcements needs to be inside the element that wraps the codemirror editor, but if it doesn't, then we could just add it to document.body instead and we would no longer need mountAnnouncer.

@pcardune pcardune requested a review from schanzer September 3, 2021 20:55
Copy link
Member

@schanzer schanzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to bring back muteAnnouncements. There were some subtle corner cases involving screenreader bug workarounds where our a11y consultant indicated this would be important. Many of those got fixed in the last 2-3 years, so we removed the uses....but I'm not convinced we won't need it again as we enter wider user testing.

@pcardune
Copy link
Collaborator Author

pcardune commented Sep 7, 2021

ok, this should do it.

@schanzer schanzer merged commit 168659f into master Sep 7, 2021
@pcardune pcardune deleted the pcardune-rework-announcements branch September 7, 2021 17:29
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.

2 participants