-
Notifications
You must be signed in to change notification settings - Fork 361
Support new division symbol (:) used by several locales #2862
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
Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +234 B (+0.05%) Total Size: 493 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (f039eec) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2862If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR2862If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR2862 |
| right: 0, | ||
| padding: `${sizing.size_120} ${sizing.size_160}`, | ||
| backgroundColor: semanticColor.core.background.base.default, | ||
| backgroundColor: "#FFFFFF", |
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.
This token was updated by wonderblocks, but this is breaking most of our storybook docs even after running pnpm install. I've just changed this to white for now as this is a testing component and I'd rather not have to maintain all the semantic colour tokens if they're still fluctuating so frequently.
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 that as a comment above this line so it's obvious when we come back to it in the future?
| "vi", | ||
| "ro", | ||
| "ru", | ||
| ]; |
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 determined these using an internal spreadsheet that breaks down the differences across locales. If you'd like the reference material, please let me know and I'll share it! My searches have indicated that there's no automated way to detect this, so we're stuck checking against a list of known locales.
| return "/"; | ||
| }; | ||
|
|
||
| export const getDivideSymbolForTex = (locale: string): string => { |
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.
The keypad enters TeX commands into our Math Inputs in order to properly display certain math formats/symbols. Unfortunately, the division symbol is one such symbol that requires TeX. (Something that wasn't required for our decimal separator)
However, our scoring logic requires that we return a forward slash. In order to support both features, I've opted to make a separate function for returning the TeX command, that uses the same logic as the simpler getDivideSymbol. I'm happy to take any alternative suggestions on this, however!
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.
Makes sense to me. It could be helpful to put this explanation in a doc comment for getDivideSymbolForTex
95fcf2e to
5ef04d1
Compare
…cording to users locale
5ef04d1 to
c692305
Compare
nishasy
left a comment
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.
Mostly suggestions about comments, nothing blocking. Looks good to me!
| export const getDivideSymbolForTex = (locale: string): string => { | ||
| const divideSymbol = getDivideSymbol(locale); | ||
|
|
||
| // If the locale uses the forward slash, return the appropriate TeX command |
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.
| // If the locale uses the forward slash, return the appropriate TeX command | |
| // If the locale uses the forward slash, return the TeX command for ÷ |
| return "/"; | ||
| }; | ||
|
|
||
| export const getDivideSymbolForTex = (locale: string): string => { |
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.
Makes sense to me. It could be helpful to put this explanation in a doc comment for getDivideSymbolForTex
| const result = scoreExpression("z:3", expressionItem3Options, "en"); | ||
| expect(result).toHaveInvalidInput(); | ||
| }); | ||
|
|
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.
It doesn't look like there are any tests for the en divide sign in this file? Could it be worth adding one? I know it's already written as z/3 in the test data, but I think it could be worth confirming that it still works in the z/3 case.
| item: expressionItem4, | ||
| }, | ||
| }; | ||
|
|
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 doc comment above this story explaining how it can be used to test division? The comment should show up above the story in Storybook as a description.
| right: 0, | ||
| padding: `${sizing.size_120} ${sizing.size_160}`, | ||
| backgroundColor: semanticColor.core.background.base.default, | ||
| backgroundColor: "#FFFFFF", |
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 that as a comment above this line so it's obvious when we come back to it in the future?
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] ### Minor Changes - [#2908](#2908) [`d60a719785`](d60a719) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (CX) (a11y) | Add long description field to Image widget editor - [#2910](#2910) [`55769ff4da`](55769ff) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing several issues related to User Input in our Explanation and Number Line Widgets ### Patch Changes - [#2887](#2887) [`1ec4595857`](1ec4595) Thanks [@handeyeco](https://github.com/handeyeco)! - Add regression test for answerless-to-answerful transition - [#2862](#2862) [`4e9b6f012d`](4e9b6f0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Add support for colon division symbol according to users locale - [#2903](#2903) [`79a6fc682b`](79a6fc6) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Fix rendering error in InputWithExamples, changing replaceAll to replace to be old browser compatible - Updated dependencies \[[`4e9b6f012d`](4e9b6f0), [`55769ff4da`](55769ff), [`4733bbb3a8`](4733bbb)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Minor Changes - [#2910](#2910) [`55769ff4da`](55769ff) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing several issues related to User Input in our Explanation and Number Line Widgets ### Patch Changes - [#2862](#2862) [`4e9b6f012d`](4e9b6f0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Add support for colon division symbol according to users locale - Updated dependencies \[[`4e9b6f012d`](4e9b6f0)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Minor Changes - [#2908](#2908) [`d60a719785`](d60a719) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (CX) (a11y) | Add long description field to Image widget editor - [#2910](#2910) [`55769ff4da`](55769ff) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing several issues related to User Input in our Explanation and Number Line Widgets ### Patch Changes - Updated dependencies \[[`d60a719785`](d60a719), [`1ec4595857`](1ec4595), [`4e9b6f012d`](4e9b6f0), [`79a6fc682b`](79a6fc6), [`55769ff4da`](55769ff), [`4733bbb3a8`](4733bbb)]: - @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 - [#2862](#2862) [`4e9b6f012d`](4e9b6f0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Add support for colon division symbol according to users locale ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`4e9b6f012d`](4e9b6f0), [`55769ff4da`](55769ff)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`4e9b6f012d`](4e9b6f0), [`55769ff4da`](55769ff)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#2862](#2862) [`4e9b6f012d`](4e9b6f0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Add support for colon division symbol according to users locale - [#2907](#2907) [`4733bbb3a8`](4733bbb) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Fix missing border in mobile keypad - Updated dependencies \[[`4e9b6f012d`](4e9b6f0), [`55769ff4da`](55769ff)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`4e9b6f012d`](4e9b6f0), [`55769ff4da`](55769ff)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#2862](#2862) [`4e9b6f012d`](4e9b6f0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Add support for colon division symbol according to users locale - Updated dependencies \[[`4e9b6f012d`](4e9b6f0), [`55769ff4da`](55769ff)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`4e9b6f012d`](4e9b6f0), [`55769ff4da`](55769ff), [`4733bbb3a8`](4733bbb)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected]
Summary:
This PR adds support for locales (particularly Ukraine) that use
:instead of/for denoting division in math equations. We're only focusing on the Expression Widget as Numeric Input / Input Number do not support the division symbol.We're currently checking with the language advocates about how fractions are written in Ukrainian, but initial research is indicating that the bar
/is still used in those cases.This PR does not update the actual icon on the keypad, as we're still waiting for design. We will update the keypad button (in the appropriate locales) in a future PR, after LEMS-3535 has been completed.
Video Example:
Screen.Recording.2025-09-03.at.2.32.06.PM.mov
Issue: LEMS-3435
Test plan: