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

[Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only) #1488

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

nishasy
Copy link
Contributor

@nishasy nishasy commented Aug 6, 2024

Summary:

Add the UI to specify start coords for Polygon graph type (not unlimited, snap to grid only)

  • Add the polygon graph type go start-coords-settings.tsx
    • No need to create a separate start-coord-polygon.tsx file because
      it's identical to start-coord-point.tsx. Reusing that component.
  • Update the selection logic in interactive-graph-editor.tsx so it
    properly updates the graph with the new snapTo value. (It was stuck
    on the previously selected value before.)
  • Add the start coords UI polygon flag
  • Update the flag tests so they actually test what they're supposed to

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

Test plan:

yarn jest

Storybook

Screenshot 2024-08-05 at 5 29 35 PM

@nishasy nishasy self-assigned this Aug 6, 2024
@@ -700,9 +700,10 @@ describe("InteractiveGraphEditor", () => {
...flags,
mafs: {
...flags.mafs,
"start-coords-ui-phase-1": shouldRender,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm so embarrassed by how broken these tests were. They basically weren't even testing anything! 😬

Copy link
Contributor

github-actions bot commented Aug 6, 2024

Size Change: +326 B (+0.04%)

Total Size: 853 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 273 kB +134 B (+0.05%)
packages/perseus/dist/es/index.js 413 kB +135 B (+0.03%)
packages/perseus/dist/es/strings.js 3.29 kB +57 B (+1.77%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.3 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.8 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-linter/dist/es/index.js 21.6 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

@nishasy nishasy marked this pull request as ready for review August 6, 2024 00:38
@khan-actions-bot khan-actions-bot requested a review from a team August 6, 2024 00:38
@khan-actions-bot
Copy link
Contributor

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/unlucky-lamps-draw.md, packages/perseus/src/index.ts, packages/perseus/src/types.ts, packages/perseus-editor/src/__stories__/flags-for-api-options.ts, packages/perseus-editor/src/components/start-coords-settings.tsx, packages/perseus-editor/src/components/util.ts, packages/perseus-editor/src/widgets/interactive-graph-editor.tsx, packages/perseus-editor/src/components/__tests__/start-coords-settings.test.tsx, packages/perseus-editor/src/components/__tests__/util.test.ts, packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx, packages/perseus/src/widgets/interactive-graphs/reducer/initialize-graph-state.ts

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

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.24%. Comparing base (0b625f5) to head (18e9c85).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1488      +/-   ##
==========================================
+ Coverage   70.12%   70.24%   +0.12%     
==========================================
  Files         510      513       +3     
  Lines      105591   105684      +93     
  Branches     7653    11427    +3774     
==========================================
+ Hits        74042    74240     +198     
- Misses      31360    31444      +84     
+ Partials      189        0     -189     

Impacted file tree graph

Files Coverage Δ
...us-editor/src/components/start-coords-settings.tsx 98.69% <100.00%> (+4.13%) ⬆️
packages/perseus-editor/src/components/util.ts 85.07% <100.00%> (+0.95%) ⬆️
...us-editor/src/widgets/interactive-graph-editor.tsx 91.28% <100.00%> (+0.03%) ⬆️
packages/perseus/src/index.ts 100.00% <100.00%> (ø)
...teractive-graphs/reducer/initialize-graph-state.ts 81.44% <100.00%> (-12.22%) ⬇️

... and 158 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b625f5...18e9c85. Read the comment docs.

Comment on lines +868 to +870
// Arrange

// Act
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is it best practice to still have both of these (arrange and act) and to have them separate like this if arrange is empty? Just curious as I usually combine them and want to make sure I do it the best way 😅

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 there's no style guide for this, but there's a preference among folks to leave the comments on separate lines.

https://khanacademy.slack.com/archives/CJSB0D3BN/p1722962522758789

Copy link
Contributor

@Myranae Myranae left a comment

Choose a reason for hiding this comment

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

Super exciting! Changes look good, but when running through the test plan, what should be different about the two stories (after changing the second one to a polygon graph)? They both have the start coords ui and the use default coords button works for both. Also confirmed the start coords ui goes away when snapTo is set to angles or sides. Looks like it's working! :)

Copy link
Contributor

github-actions bot commented Aug 6, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1488

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

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

@nishasy
Copy link
Contributor Author

nishasy commented Aug 6, 2024

@Myranae ah, I should have clarified. The difference between the two cases in the test plan is that one make sure that the UI shows up when start coords have already been passed in, and the other makes sure the UI shows up by default when freshly initializing a graph. I had an issue during development where one worked and the other didn't, so I wanted to add that to the test plan explicitly.

@nishasy nishasy merged commit 0bec013 into main Aug 6, 2024
11 checks passed
@nishasy nishasy deleted the start-coords-ui-polygon branch August 6, 2024 16:56
Myranae pushed a commit that referenced this pull request Aug 9, 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]

### Major Changes

-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead

### Minor Changes

-   [#1455](#1455) [`8b0268215`](8b02682) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon graphs to have weighted lines only on keyboard focus


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1428](#1428) [`eb9f3f9c0`](eb9f3f9) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Drop katex dependency - no longer used


-   [#1472](#1472) [`4c2ace57a`](4c2ace5) Thanks [@benchristel](https://github.com/benchristel)! - Add keyboard controls to Mafs angle graphs


-   [#1487](#1487) [`53031f8df`](53031f8) Thanks [@Myranae](https://github.com/Myranae)! - Make graphs non-interactive via keyboard when their question has been answered correctly


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1474](#1474) [`a8aaac339`](a8aaac3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor type fixes in the `ImageLoader` and `SvgImage` components


-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1475](#1475) [`6a218bcc1`](6a218bc) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - add non-visual text as a description for expression widget

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Minor Changes

-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1491](#1491) [`395b44f29`](395b44f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Correct flag logic for polygon graph start coords UI


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1471](#1471) [`b4e0b60dc`](b4e0b60) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Fix the dropdowns overlapping labels in locked figures settings


-   [#1481](#1481) [`a35be719f`](a35be71) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Stop page scrolling on number text field focused scroll


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1477](#1477) [`653b520c6`](653b520) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Migrate ItemExtrasEditor to WB checkboxes

-   Updated dependencies \[[`a8aaac339`](a8aaac3), [`de13fd337`](de13fd3), [`8b0268215`](8b02682), [`2ebbc1978`](2ebbc19), [`6c6ff52f4`](6c6ff52), [`18f38fca4`](18f38fc), [`ef96cdd54`](ef96cdd), [`342a72211`](342a722), [`de2883b3f`](de2883b), [`7fd586ef4`](7fd586e), [`f920a4cc7`](f920a4c), [`eb9f3f9c0`](eb9f3f9), [`4c2ace57a`](4c2ace5), [`53031f8df`](53031f8), [`808956098`](8089560), [`5e66539e6`](5e66539), [`0b625f560`](0b625f5), [`6a218bcc1`](6a218bc), [`0bec013e8`](0bec013)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

-   [#1495](#1495) [`6c6ff52f4`](6c6ff52) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove old buttons that we weren't using anymore


-   [#1475](#1475) [`342a72211`](342a722) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update wonder blocks popover versions


-   [#1496](#1496) [`5e66539e6`](5e66539) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove unused buttons from MathInput; add Lato

## @khanacademy/[email protected]

### Patch Changes

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

Author: khan-actions-bot

Reviewers: Myranae

Required Reviewers:

Approved By: Myranae

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️  Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1473
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