-
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
[SR] Circle - Add the interactive elements circle description to the whole graph container #2060
Changes from 3 commits
415ce10
84485df
1c1c88e
d7add26
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": patch | ||
--- | ||
|
||
[SR] Circle - Add interactive Circle element to full graph description |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import { | |
useTransformVectorsToPixels, | ||
} from "./use-transform"; | ||
|
||
import type {I18nContextType} from "../../../components/i18n-context"; | ||
import type { | ||
AriaLive, | ||
CircleGraphState, | ||
|
@@ -30,7 +31,7 @@ export function renderCircleGraph( | |
): InteractiveGraphElementSuite { | ||
return { | ||
graph: <CircleGraph graphState={state} dispatch={dispatch} />, | ||
interactiveElementsDescription: null, | ||
interactiveElementsDescription: CircleGraphDescription({state}), | ||
}; | ||
} | ||
|
||
|
@@ -51,40 +52,25 @@ function CircleGraph(props: CircleGraphProps) { | |
const outerPointsId = id + "-outer-points"; | ||
|
||
// Aria label strings | ||
const circleGraphAriaLabel = strings.srCircleGraph; | ||
const circleShapeAriaLabel = strings.srCircleShape({ | ||
centerX: srFormatNumber(center[0], locale), | ||
centerY: srFormatNumber(center[1], locale), | ||
}); | ||
const circleRadiusPointAriaLabel = strings.srCircleRadiusPoint({ | ||
radiusPointX: srFormatNumber(radiusPoint[0], locale), | ||
radiusPointY: srFormatNumber(radiusPoint[1], locale), | ||
}); | ||
const circleRadiusDescription = strings.srCircleRadius({ | ||
radius, | ||
}); | ||
const circleOuterPointsDescription = strings.srCircleOuterPoints({ | ||
point1X: srFormatNumber(center[0] + radius, locale), | ||
point1Y: srFormatNumber(center[1], locale), | ||
point2X: srFormatNumber(center[0], locale), | ||
point2Y: srFormatNumber(center[1] + radius, locale), | ||
point3X: srFormatNumber(center[0] - radius, locale), | ||
point3Y: srFormatNumber(center[1], locale), | ||
point4X: srFormatNumber(center[0], locale), | ||
point4Y: srFormatNumber(center[1] - radius, locale), | ||
}); | ||
const { | ||
srCircleGraph, | ||
srCircleShape, | ||
srCircleRadiusPoint, | ||
srCircleRadius, | ||
srCircleOuterPoints, | ||
} = describeCircleGraph(graphState, {strings, locale}); | ||
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 like this way of splitting up the logic! Makes it much easier to read and test :) |
||
|
||
return ( | ||
<g | ||
// Outer circle minimal description | ||
aria-label={circleGraphAriaLabel} | ||
aria-label={srCircleGraph} | ||
aria-describedby={`${circleId} ${radiusId} ${outerPointsId}`} | ||
> | ||
<MovableCircle | ||
id={circleId} | ||
// Focusable circle aria label reads with every update | ||
// because of the aria-live property in the circle <g>. | ||
ariaLabel={circleShapeAriaLabel} | ||
ariaLabel={srCircleShape} | ||
// Aria-describedby describes additional info on focus. | ||
ariaDescribedBy={`${radiusId} ${outerPointsId}`} | ||
center={center} | ||
|
@@ -96,7 +82,7 @@ function CircleGraph(props: CircleGraphProps) { | |
/> | ||
<MovablePoint | ||
// Radius point aria label reads with every update. | ||
ariaLabel={`${circleRadiusPointAriaLabel} ${circleRadiusDescription}`} | ||
ariaLabel={`${srCircleRadiusPoint} ${srCircleRadius}`} | ||
// Aria-describedby describes additional info on focus. | ||
ariaDescribedBy={`${outerPointsId}`} | ||
// The radius point's aria-live property is set to "off" when | ||
|
@@ -116,10 +102,10 @@ function CircleGraph(props: CircleGraphProps) { | |
{/* Hidden elements to provide the descriptions for the | ||
circle and radius point's `aria-describedby` properties. */} | ||
<g id={radiusId} style={{display: "hidden"}}> | ||
{circleRadiusDescription} | ||
{srCircleRadius} | ||
</g> | ||
<g id={outerPointsId} style={{display: "hidden"}}> | ||
{circleOuterPointsDescription} | ||
{srCircleOuterPoints} | ||
</g> | ||
</g> | ||
); | ||
|
@@ -221,3 +207,60 @@ function crossProduct<A, B>(as: A[], bs: B[]): [A, B][] { | |
} | ||
return result; | ||
} | ||
|
||
function CircleGraphDescription({state}: {state: CircleGraphState}) { | ||
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. this is the same as 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 that works too. I think the reason it was done this way (I was following what Ben did for the point description) is so it follows the prop structure the way they're standardly written for other functional components. 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. Yeah, here's the real reason: |
||
// The reason that CircleGraphDescription is a component (rather than a | ||
// function that returns a string) is because it needs to use a | ||
// hook: `usePerseusI18n`. | ||
const i18n = usePerseusI18n(); | ||
const strings = describeCircleGraph(state, i18n); | ||
|
||
return strings.srCircleInteractiveElement; | ||
} | ||
|
||
// Exported for testing | ||
export function describeCircleGraph( | ||
state: CircleGraphState, | ||
i18n: I18nContextType, | ||
): Record<string, string> { | ||
const {strings, locale} = i18n; | ||
const {center, radiusPoint} = state; | ||
const radius = getRadius(state); | ||
|
||
// Aria label strings | ||
const srCircleGraph = strings.srCircleGraph; | ||
const srCircleShape = strings.srCircleShape({ | ||
centerX: srFormatNumber(center[0], locale), | ||
centerY: srFormatNumber(center[1], locale), | ||
}); | ||
const srCircleRadiusPoint = strings.srCircleRadiusPoint({ | ||
radiusPointX: srFormatNumber(radiusPoint[0], locale), | ||
radiusPointY: srFormatNumber(radiusPoint[1], locale), | ||
}); | ||
const srCircleRadius = strings.srCircleRadius({ | ||
radius, | ||
}); | ||
const srCircleOuterPoints = strings.srCircleOuterPoints({ | ||
point1X: srFormatNumber(center[0] + radius, locale), | ||
point1Y: srFormatNumber(center[1], locale), | ||
point2X: srFormatNumber(center[0], locale), | ||
point2Y: srFormatNumber(center[1] + radius, locale), | ||
point3X: srFormatNumber(center[0] - radius, locale), | ||
point3Y: srFormatNumber(center[1], locale), | ||
point4X: srFormatNumber(center[0], locale), | ||
point4Y: srFormatNumber(center[1] - radius, locale), | ||
}); | ||
|
||
const srCircleInteractiveElement = strings.srInteractiveElements({ | ||
elements: [srCircleShape, srCircleRadius].join(" "), | ||
}); | ||
|
||
return { | ||
srCircleGraph, | ||
srCircleShape, | ||
srCircleRadiusPoint, | ||
srCircleRadius, | ||
srCircleOuterPoints, | ||
srCircleInteractiveElement, | ||
}; | ||
} |
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.
I'm slightly surprised that this works. Since
CircleGraphDescription
is calling a hook, I'd expect it to have to be used as a JSX element:If the component calling
renderCircleGraph
does so conditionally, e.g.And
graphType
changes between renders, this code will break the Rules of Hooks. TheusePerseusI18n
hook will be called on one render and not called on the next.I think it's working (mostly?) because the graph type rarely changes between renders. It probably only does so in the content editor.
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.
Ack! My mistake. I 100% meant to use your code for Point as the blueprint for this, which means I was supposed to write
<CircleGraphDescription state={state} />
.