-
Notifications
You must be signed in to change notification settings - Fork 350
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
feat(LEMS-1733): adds aria labels to line segment #2032
base: main
Are you sure you want to change the base?
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (0616ef8) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2032 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2032 |
Size Change: +386 B (+0.03%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of small requests.
context: | ||
"Screenreader-only description of a line segment on a coordinate plane.", | ||
message: | ||
"Endpoint 1 at %(point1X)s comma %(point1Y)s. Endpoint 2 %(point2X)s comma %(point2Y)s. Segment length %(length)s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put a period at the end of this sentence? It can sound weird when there are multiple sentences and one of them doesn't have it when the screen reader is reading them back to back.
Also, I think this one is supposed to end with "Segment length %(length)s units." according to the SRUX doc.
srSegmentGraphEndpointAriaLabel: { | ||
context: | ||
"Screenreader-accessible label for an endpoint of a line segment on a coordinate plane.", | ||
message: "Endpoint $(endpointNumber)s at $(x)s comma $(y)s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a period at the end of this one too?
point2X: srFormatNumber(segments[0][1][X], locale), | ||
point2Y: srFormatNumber(segments[0][1][Y], locale), | ||
length: srFormatNumber(lengthOfSegment, locale), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: I know why this is happening. You have to use unique IDs to fix this. You can see how I did this for circle and linear if you need a reference.
I'm not really sure what's going on, but it seems like this is somehow using the correct answer or default coordinates instead of the ones that are on the graph? I'm extra confused because your implementation looks right to me... 🤔
It has the right values for the points themselves, just not for the aria describedby.
Screen.Recording.2024-12-18.at.5.04.27.PM.mov
/> | ||
))} | ||
</> | ||
<g id="segment-description" style={{display: "hidden"}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use React.useId()
to create unique IDs. If there are ever multiple segment interactive graphs on the screen, they'd both read the description for the first one.
OHH!!! I JUST FIGURED OUT THE PROBLEM FROM THE ABOVE COMMENT WHILE WRITING THIS!
Summary:
Issue: LEMS-1733
Test plan:
Expected behavior
A line segment on a coordinate plane
Endpoint 1 at ${point1X} comma ${point1Y}. Endpoint 2 ${point2X} comma ${point2Y}. Segment length %(length)s
Endpoint ${endpointNumber} at ${x} comma ${y}
Screenshots: