-
Notifications
You must be signed in to change notification settings - Fork 353
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
[Locked Figure Labels] Util function to generate spoken math + use it within Locked Point aria labels #1839
Changes from 6 commits
096a79d
a3bef8a
144b937
d00f96d
4fc07d8
d2139d6
007dbfa
8872044
7916eca
44cc790
2662370
fc8c21d
af82ae2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/perseus-editor": patch | ||
--- | ||
|
||
[Locked Figure Labels] Util function to generate spoken math + use it within Locked Point aria labels |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -24,6 +24,7 @@ import LockedFigureSettingsActions from "./locked-figure-settings-actions"; | |||||||||
import LockedLabelSettings from "./locked-label-settings"; | ||||||||||
import { | ||||||||||
generateLockedFigureAppearanceDescription, | ||||||||||
generateSpokenMathDetails, | ||||||||||
getDefaultFigureForType, | ||||||||||
} from "./util"; | ||||||||||
|
||||||||||
|
@@ -104,28 +105,39 @@ const LockedPointSettings = (props: Props) => { | |||||||||
|
||||||||||
const isDefiningPoint = !onMove && !onRemove; | ||||||||||
|
||||||||||
/** | ||||||||||
* Get a prepopulated aria label for the point. | ||||||||||
* | ||||||||||
* If the point has no labels, the aria label will just be | ||||||||||
* "Point at (x, y)". | ||||||||||
* | ||||||||||
* If the point has labels, the aria label will be | ||||||||||
* "Point at (x, y) with label1, label2, label3". | ||||||||||
*/ | ||||||||||
function getPrepopulatedAriaLabel() { | ||||||||||
let visiblelabel = ""; | ||||||||||
if (labels && labels.length > 0) { | ||||||||||
visiblelabel += ` ${labels.map((l) => l.text).join(", ")}`; | ||||||||||
} | ||||||||||
let str = `Point${visiblelabel} at (${coord[0]}, ${coord[1]})`; | ||||||||||
const [prepopulatedAriaLabel, setPrepopulatedAriaLabel] = | ||||||||||
React.useState(""); | ||||||||||
|
||||||||||
React.useEffect(() => { | ||||||||||
/** | ||||||||||
* Get a prepopulated aria label for the point, with the math | ||||||||||
* details converted into spoken words. | ||||||||||
* | ||||||||||
* If the point has no labels, the aria label will just be | ||||||||||
* "Point at (x, y)". | ||||||||||
* | ||||||||||
* If the point has labels, the aria label will be | ||||||||||
* "Point at (x, y) with label1, label2, label3". | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment should be something like
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! |
||||||||||
*/ | ||||||||||
async function getPrepopulatedAriaLabel() { | ||||||||||
let visiblelabel = ""; | ||||||||||
if (labels && labels.length > 0) { | ||||||||||
visiblelabel += ` ${labels.map((l) => l.text).join(", ")}`; | ||||||||||
} | ||||||||||
|
||||||||||
const pointAppearance = | ||||||||||
generateLockedFigureAppearanceDescription(pointColor); | ||||||||||
str += pointAppearance; | ||||||||||
let str = await generateSpokenMathDetails( | ||||||||||
`Point${visiblelabel} at (${coord[0]}, ${coord[1]})`, | ||||||||||
); | ||||||||||
|
||||||||||
return str; | ||||||||||
} | ||||||||||
const pointAppearance = | ||||||||||
generateLockedFigureAppearanceDescription(pointColor); | ||||||||||
str += pointAppearance; | ||||||||||
|
||||||||||
return str; | ||||||||||
} | ||||||||||
|
||||||||||
getPrepopulatedAriaLabel().then(setPrepopulatedAriaLabel); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a race condition here because the last I think we could guard against that race condition by doing this in the useEffect callback: let canceled = false;
getPrepopulatedAriaLabel().then((label) => {
!canceled && setPrepopulatedAriaLabel(label);
})
return () => canceled = true; The annoying thing is that this is really hard to test. What we really want here is something like a Then we could just write this: const prepopulatedAriaLabel = useAsyncMemo(
/* initial value */ "",
() => getPrepopulatedAriaLabel(coord, labels, pointColor),
[coord, labels, pointColor],
); |
||||||||||
}, [coord, labels, pointColor]); | ||||||||||
|
||||||||||
function handleColorChange(newValue) { | ||||||||||
const newProps: Partial<LockedPointType> = { | ||||||||||
|
@@ -247,7 +259,7 @@ const LockedPointSettings = (props: Props) => { | |||||||||
|
||||||||||
<LockedFigureAria | ||||||||||
ariaLabel={ariaLabel} | ||||||||||
prePopulatedAriaLabel={getPrepopulatedAriaLabel()} | ||||||||||
prePopulatedAriaLabel={prepopulatedAriaLabel} | ||||||||||
onChangeProps={(newProps) => { | ||||||||||
onChangeProps(newProps); | ||||||||||
}} | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,10 @@ type ParsedNode = { | |
function condenseTextNodes(nodes: Array<ParsedNode>): Array<ParsedNode> { | ||
const result: ParsedNode[] = []; | ||
|
||
if (!nodes) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the types, this condition should never be met, because |
||
return result; | ||
} | ||
|
||
let currentText = ""; | ||
for (const node of nodes) { | ||
if (node.type === "math") { | ||
|
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.
Since the actual generateSpokenMathDetails function is async, do we want to return a Promise here to make sure the result is correctly being awaited? (Or maybe we trust TypeScript to tell us if we get that wrong?)
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.
That's a great point! I'll change it to the promise.