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

Add TeX support to dropdown #1810

Merged
merged 19 commits into from
Dec 18, 2024
Merged

Add TeX support to dropdown #1810

merged 19 commits into from
Dec 18, 2024

Conversation

daniellewhyte
Copy link
Contributor

@daniellewhyte daniellewhyte commented Oct 30, 2024

Summary:

This PR adds support for showing TeX in the dropdown

Issue: LIT-1425

Test plan:

Inspect new story:
http://localhost:6006/?path=/story/perseus-widgets-dropdown--dropdown-with-math

  • Valid TeX renders in OptionItems when dropdown is expanded
Screenshot 2024-12-13 at 1 41 39 PM
  • Valid TeX renders when an option is selected
Screenshot 2024-12-13 at 1 41 45 PM
  • aria-labels exist on the mathjax-wrapper -- HTML inspection. SR reads the labels fine in the list box, but I've had mixed results after an option is selected (works on Firefox, but not Safari). This is not unique to TeX content, and there are a couple issues to address it here: LIT-1516 WB-1799
Screenshot 2024-12-13 at 1 49 23 PM

@daniellewhyte daniellewhyte self-assigned this Oct 30, 2024
Copy link
Contributor

github-actions bot commented Oct 30, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1810

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

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

Copy link
Contributor

github-actions bot commented Oct 30, 2024

Size Change: +44 B (0%)

Total Size: 1.27 MB

Filename Size Change
packages/perseus/dist/es/index.js 416 kB +44 B (+0.01%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 39 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 78 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 688 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/perseus/dist/es/strings.js 4.12 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.5 kB

compressed-size-action

@daniellewhyte
Copy link
Contributor Author

This will be ready for review once this update has been released to WB: Khan/wonder-blocks#2354

daniellewhyte added a commit to Khan/wonder-blocks that referenced this pull request Dec 2, 2024
## Summary:
When you pass in a JSX Element as a label to `OptionItem`, the SelectOpener is labeled with an empty string. This PR updates SelectOpener in the `SingleSelect` and `MultiSelect` components to return the JSX as a label in that case. 

This change is being made to unblock supporting TEX in the Perseus Dropdown widget. 

Issue: LIT-1425


## Test plan:
- Added new stories
  - https://5e1bf4b385e3fb0020b7073c-xhxyrfwkfd.chromatic.com/?path=/story/packages-dropdown-singleselect--custom-option-item-with-node-label
  - https://5e1bf4b385e3fb0020b7073c-xhxyrfwkfd.chromatic.com/?path=/story/packages-dropdown-multiselect--custom-option-items-with-node-label
- Unit tests pass
- Installed npm snapshot and tested against my branch in Perseus ([PR here](Khan/perseus#1810))
- Test in webapp and storybook to ensure no regressions

Author: daniellewhyte

Reviewers: marcysutton, daniellewhyte, beaesguerra, jandrade

Required Reviewers:

Approved By: beaesguerra, jandrade

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️  dependabot

Pull Request URL: #2354
@daniellewhyte daniellewhyte changed the title [WIP] add TeX support to dropdown Add TeX support to dropdown Dec 11, 2024
@daniellewhyte daniellewhyte marked this pull request as ready for review December 13, 2024 19:40
@daniellewhyte daniellewhyte marked this pull request as draft December 13, 2024 21:12
@daniellewhyte daniellewhyte marked this pull request as ready for review December 16, 2024 21:25
@daniellewhyte daniellewhyte requested a review from a team December 16, 2024 21:26
@@ -1,6 +1,6 @@
import type {PerseusRenderer} from "../../perseus-types";

export const question1: PerseusRenderer = {
export const basicDropdown: PerseusRenderer = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!! Meaningful names are so hard to come by these days.

content={this.props.placeholder}
strings={this.context.strings}
inline
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Using <Renderer> feels heavy for rendering TeX. Perhaps using the <TeX> component would be a lighter-weight implementation? Access to this component is via getDependencies() instead of direct import. Can we give that a try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that component always renders everything as TeX.

Screenshot 2024-12-17 at 2 34 05 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Bummer. OK. Let's stick with the <Renderer> widget for now. Thanks for checking into it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mark and I chatted in DMs, but I'll circle back here. I suggested the TeX component, but didn't realize there might be a mixture of standard content and TeX markup. In that case, Renderer is the correct tool. It handles parsing content and rendering TeX as needed. This is pretty common to use the Renderer in widgets to render content (even if widgets aren't supported).

@catandthemachines
Copy link
Member

I think there is a typo in your code somewhere, it reads out the same option 5x for all the options.
image

@catandthemachines
Copy link
Member

I think there is a typo in your code somewhere, it reads out the same option 5x for all the options. image

This actually might be an issue with Chrome, as Safari doesn't do this. I would say this is non-blocking feedback, might go away if you use the component.

@daniellewhyte
Copy link
Contributor Author

@catandthemachines I think you're right that it's a VO+Chrome issue. I am able to recreate the same behavior on the MathJax stories. It doesn't read the square root.

Screenshot 2024-12-17 at 3 02 58 PM

@daniellewhyte daniellewhyte merged commit e21ead8 into main Dec 18, 2024
13 of 18 checks passed
@daniellewhyte daniellewhyte deleted the dropdown-tex-support branch December 18, 2024 15:08
daniellewhyte added a commit that referenced this pull request Dec 18, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- [#1810](#1810)
[`e21ead80e`](e21ead8)
Thanks [@daniellewhyte](https://github.com/daniellewhyte)! - Update
Dropdown widget to support displaying TeX

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- [#1810](#1810)
[`e21ead80e`](e21ead8)
Thanks [@daniellewhyte](https://github.com/daniellewhyte)! - Update
Dropdown widget to support displaying TeX


- [#2023](#2023)
[`51386d6e0`](51386d6)
Thanks [@nishasy](https://github.com/nishasy)! - [SR] Linear graph - add
full graph aria label and description

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- [#1810](#1810)
[`e21ead80e`](e21ead8)
Thanks [@daniellewhyte](https://github.com/daniellewhyte)! - Update
Dropdown widget to support displaying TeX

## @khanacademy/[email protected]

### Patch Changes

- [#1810](#1810)
[`e21ead80e`](e21ead8)
Thanks [@daniellewhyte](https://github.com/daniellewhyte)! - Update
Dropdown widget to support displaying TeX

- Updated dependencies
\[[`e21ead80e`](e21ead8),
[`51386d6e0`](51386d6)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- [#1810](#1810)
[`e21ead80e`](e21ead8)
Thanks [@daniellewhyte](https://github.com/daniellewhyte)! - Update
Dropdown widget to support displaying TeX

- Updated dependencies
\[[`e21ead80e`](e21ead8)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
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.

4 participants