Announcer: Part 1 #2362
Announcer: Part 1 #2362
Conversation
🦋 Changeset detectedLatest commit: 2428b60 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +1.55 kB (+1.54%) Total Size: 102 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-onaptwedxt.chromatic.com/ Chromatic results:
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (1053704) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2362"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2362 |
There was a problem hiding this comment.
suggestion: you can get rid of this file as changesets will create it automatically when the package is released to npm.
There was a problem hiding this comment.
suggestion: This seems to be only used for tests. In that case, I'd suggest to move it to the test file directly. This will help us understand better what's part of the API and what is internal tooling.
| * @param {number} removalDelay Optional duration to remove a message after sending. Defaults to 5000ms. | ||
| * @returns {string} IDREF for targeted live region element | ||
| */ | ||
| export function sendMessage({ |
There was a problem hiding this comment.
suggestion: I understand that this is the first iteration. I'd suggest moving this into a separate file so the code is more isolated (and tests can be isolated as you do it already).
I'd suggest doing something similar with clearMessages.
You can see how other packages follow this structure (for example, wb-core).
There was a problem hiding this comment.
One consideration for this is I was using a shared variable for the announcer in both of those API methods. I will lose that variable access when moving to separate files. One solution would be to use a global on the window object. Do you have any objections or suggestions for how to work around that?
| if (typeof jest === "undefined") { | ||
| setTimeout(() => { | ||
| return announcer?.announce(message, level, removalDelay); | ||
| }, 100); | ||
| } else { | ||
| // If we are in a test environment, announce without waiting | ||
| return announcer.announce(message, level, removalDelay); | ||
| } |
There was a problem hiding this comment.
suggestion: I'd strongly recommend not using test checks in the prod code. Instead, you could probably use advanceTimersByTime in during test setup.
e.g.
// Act
do something to trigger the announcer
jest.advanceTimersByTime(100);
For reference: https://jestjs.io/docs/timer-mocks#advance-timers-by-time
You can see some existing examples in the codebase.
| setTimeout(() => { | ||
| return announcer?.announce(message, level, removalDelay); | ||
| }, 100); |
There was a problem hiding this comment.
question: Is there a specific reason why we need to use this timeout? I wonder if there's another way to handle this case, but I don't think I have full context on that.
There was a problem hiding this comment.
It was for Safari and VoiceOver, but I can play around with it again to see if we really need it.
|
|
||
| private constructor() { | ||
| if (typeof document !== "undefined") { | ||
| const topLevelId = `wbAnnounce`; |
There was a problem hiding this comment.
suggestion: This is a good candidate for a constant at the file level so this ID can be shared across the file. wdyt?
There was a problem hiding this comment.
That's what I had at first, but it added cognitive load when working with the API. It makes the file a lot more scannable to locate this ID closer to the code it's used on!
| Object.assign(this.node.style, srOnly); | ||
|
|
||
| const aWrapper = this.createRegionWrapper("assertive"); | ||
| this.createDuplicateRegions(aWrapper, "assertive"); |
There was a problem hiding this comment.
question: Could you please elaborate why we need to create duplicate regions here? I was looking at the story in the browser and I'm not sure what should expect from that.
There was a problem hiding this comment.
It's an architectural decision to try and prevent messages from being dropped by AT, something I picked up back in the Angular.js days. Coupled with the debounce logic I'm working on, I think I'll keep it in for now to ensure unique messages are appended and always picked up by AT.
| /** | ||
| * Styling for live region. | ||
| * TODO: move to wonder-blocks-style package. | ||
| * Note: This style is overridden in Storybook for testing. |
There was a problem hiding this comment.
praise: Thanks for adding this note 🫶
| /** | ||
| * Internal class to manage screen reader announcements. | ||
| */ | ||
| class Announcer { |
There was a problem hiding this comment.
suggestion: I'd recommend moving this class into a separate file as well. Sometimes we use an internal/ folder for things like this, but up to you to decide what folder is best.
Side question: Do you think that Announcer can be tested independently? or is it better to rely on the exposed APIs for unit testing (the ones that you already created)?
There was a problem hiding this comment.
Good question about testing! It seems redundant to test the class itself, at least the parts that affect the DOM. But perhaps some unit testing for the singleton and some of the utility methods would be a good idea?
There was a problem hiding this comment.
suggestion: Related to some of the comments left in this file. For consistency, we recommend splitting this file into separate files and keep this index.ts file as a barrel file. To clarify, I'm 100% with you about avoiding barrel files, but in the WB case, it really helps to make more clear and explicit what's going to be public API and what's internal/private. You can check other packages to see how we usually use this approach.
The initial implementation for a live region component! I'm getting this draft PR up so I can test with the remote URL.
Jira Issue: https://khanacademy.atlassian.net/browse/WB-1768
Outstanding questions/work areas:
sendMessageto ensure clearing messages works. This doesn't seem super critical since messages get cleared after 5000ms (and this is configurable), but I want to make sure there aren't any bugs. The Jest test for it passes but the ID returned is empty in Storybook 🤷♀️