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

Let interactive graph components render a screenreader description #1815

Merged
merged 16 commits into from
Oct 31, 2024

Conversation

benchristel
Copy link
Member

This refactoring will make it easy for each graph subtype (segment, point,
linear-system, etc.) to describe itself to a screenreader.

At this point, no graph actually renders a screenreader description. Future
PRs will add descriptions for each graph type.

Issue: https://khanacademy.atlassian.net/browse/LEMS-1725

Test plan:

yarn test

@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Oct 31, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/chatty-gorillas-shout.md, packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx, packages/perseus/src/widgets/interactive-graphs/types.ts, packages/perseus/src/widgets/interactive-graphs/graphs/angle.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/circle.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/linear-system.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/quadratic.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/ray.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/segment.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

@khan-actions-bot khan-actions-bot requested a review from a team October 31, 2024 17:57
Copy link
Contributor

github-actions bot commented Oct 31, 2024

Size Change: +203 B (+0.02%)

Total Size: 857 kB

Filename Size Change
packages/perseus/dist/es/index.js 408 kB +203 B (+0.05%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.8 kB
packages/keypad-context/dist/es/index.js 760 B
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 77.8 kB
packages/math-input/dist/es/strings.js 1.79 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-editor/dist/es/index.js 282 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/perseus/dist/es/strings.js 3.49 kB
packages/pure-markdown/dist/es/index.js 3.66 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link
Contributor

github-actions bot commented Oct 31, 2024

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (9e13cc0) and published it to npm. You
can install it using the tag PR1815.

Example:

yarn add @khanacademy/perseus@PR1815

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1815

screenreaderDescription: null,
};
}

export function AngleGraph(props: AngleGraphProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all these graphs are no longer being rendered directly outside their respective files (<AngleGraph /> ==> renderAngleGraph()), can we remove the export for all of these?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's an index.ts file in this directory that exports most of the graph components. Perhaps we should remove that. If we did, then I think this wouldn't need to be exported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the index.ts file only for exporting things that would be used outside this package? I feel like we could probably remove that, but I don't think that should block this in any case.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that nothing imports from the index.ts file, so I'm going to remove it.

Base automatically changed from benc/movable-point-aria to main October 31, 2024 20:49
@benchristel benchristel force-pushed the benc/render-graph-refactor branch from 91ae0da to 368ac54 Compare October 31, 2024 21:53
Copy link
Contributor

@nishasy nishasy left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@benchristel benchristel merged commit 2c40219 into main Oct 31, 2024
9 checks passed
@benchristel benchristel deleted the benc/render-graph-refactor branch October 31, 2024 22:15
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.

3 participants