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

Correct how arrowhead rotation is calculated #1271

Merged

Conversation

mark-fitzgerald
Copy link
Contributor

@mark-fitzgerald mark-fitzgerald commented May 15, 2024

Summary:

Arrowhead rotation (aka "direction") was calculated using graph coordinates. While this works properly most of the time, there are situations where it doesn't. Specifically, when grid units are displayed more rectangularly than as a regular square. This code change resolves that issue by using pixel coordinates, instead.

Issue: LEMS-1996

Test plan:

  1. Launch the Dev environment (yarn dev)
  2. Go to the flipbook (http://localhost:5173/#flipbook)
  3. Paste in the following graph definition:
{"content":"[[☃ interactive-graph 1]]\n\n\n","images":{},"widgets":{"input-number 1":{"alignment":"default","graded":true,"options":{"answerType":"number","inexact":false,"maxError":0.1,"simplify":"required","size":"small","value":"2.6666666666666665"},"type":"input-number","version":{"major":0,"minor":0}},"interactive-graph 1":{"alignment":"default","graded":true,"options":{"backgroundImage":{"bottom":0,"height":0,"left":0,"scale":1,"url":null,"width":0},"correct":{"coords":[[0,0],[3,8]],"type":"linear"},"graph":{"type":"linear"},"gridStep":[1,1],"labels":["t","d"],"markings":"graph","range":[[-10,1],[-10,10]],"rulerLabel":"","rulerTicks":10,"showProtractor":false,"showRuler":false,"snapStep":[0.5,0.5],"step":[1,1]},"type":"interactive-graph","version":{"major":0,"minor":0}}}}
  1. Move one of the line points around and note that the arrowheads appear to align properly with the line.

Affected behavior:

Before

Arrowhead Rotation - Before

After

Arrowhead Rotation - After

…nstead of the graph coordinates.

Correct the usage of the angle for the arrowhead (used to be using the negative value of the given angle).
Update snapshots to account for corrected angle values.
@mark-fitzgerald mark-fitzgerald self-assigned this May 15, 2024
Copy link
Contributor

github-actions bot commented May 15, 2024

Size Change: +32 B (0%)

Total Size: 839 kB

Filename Size Change
packages/perseus/dist/es/index.js 403 kB +32 B (+0.01%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.1 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.5 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 908 B
packages/perseus-editor/dist/es/index.js 268 kB
packages/perseus-error/dist/es/index.js 877 B
packages/perseus-linter/dist/es/index.js 21.8 kB
packages/perseus/dist/es/strings.js 3.22 kB
packages/pure-markdown/dist/es/index.js 3.68 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.23%. Comparing base (fba227f) to head (1a4ee60).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1271      +/-   ##
==========================================
+ Coverage   68.88%   70.23%   +1.34%     
==========================================
  Files         477      481       +4     
  Lines      101842   101915      +73     
  Branches     5138    10293    +5155     
==========================================
+ Hits        70151    71576    +1425     
+ Misses      31576    30339    -1237     
+ Partials      115        0     -115     

Impacted file tree graph

Files Coverage Δ
...eus/src/widgets/interactive-graphs/axis-arrows.tsx 100.00% <100.00%> (ø)
...interactive-graphs/graphs/components/arrowhead.tsx 100.00% <100.00%> (ø)
...ts/interactive-graphs/graphs/components/vector.tsx 100.00% <100.00%> (ø)
...eus/src/widgets/interactive-graphs/locked-line.tsx 99.30% <100.00%> (+0.03%) ⬆️

... and 133 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 fba227f...1a4ee60. Read the comment docs.

@mark-fitzgerald mark-fitzgerald requested a review from a team May 15, 2024 18:39
@@ -4185,7 +4185,7 @@ exports[`snapshots should render correctly with locked lines 1`] = `
/>
<g
class="interactive-graph-arrowhead"
transform="translate(-200 153.33333333333331) rotate(-192.52880770915152)"
transform="translate(-200 153.33333333333331) rotate(167.47119229084848)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually the same angle: -192.52880770915152 + 360 = 167.47119229084848

@@ -330,7 +330,7 @@ exports[`Rendering Does render extensions of line when option is enabled 1`] = `
/>
<g
class="interactive-graph-arrowhead"
transform="translate(0 0) rotate(-45)"
transform="translate(0 0) rotate(0)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line associated with these arrowheads are drawn with weird point references ([0,0], [0,0]) (see preceding <line> element). This line has no slope, so the rotation should be zero.

@mark-fitzgerald mark-fitzgerald marked this pull request as ready for review May 15, 2024 19:06
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented May 15, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/kind-shirts-sit.md, packages/perseus/src/widgets/__stories__/interactive-graph-regression.stories.tsx, packages/perseus/src/widgets/interactive-graphs/axis-arrows.tsx, packages/perseus/src/widgets/interactive-graphs/locked-line.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/arrowhead.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/vector.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/__snapshots__/movable-line.test.tsx.snap

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

Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

LGTM!

An idea: it might be good to add a visual regression test for this bug, in interactive-graph-regression.stories.tsx. To do that, you'd probably need to add a new InteractiveFigureConfig subtype for linear graphs in interactive-graph-question-builder.ts. LMK if you'd like to pair on that.

Copy link
Contributor

github-actions bot commented May 17, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1271

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

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

@mark-fitzgerald mark-fitzgerald merged commit 55039a4 into main May 20, 2024
12 of 13 checks passed
@mark-fitzgerald mark-fitzgerald deleted the LEMS-1996-arrowhead-rotation-incorrect-on-some-graphs branch May 20, 2024 21:41
benchristel pushed a commit that referenced this pull request May 21, 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/perseus@22.6.0

### Minor Changes

-   [#1293](#1293) [`e14a003be`](e14a003) Thanks [@benchristel](https://github.com/benchristel)! - Fill Mafs interactive circles with blue on hover


-   [#1241](#1241) [`a0dfc66cc`](a0dfc66) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - New Axis Tick Labels and Ticks that can render outside of graph bounds

### Patch Changes

-   [#1289](#1289) [`42c0c607f`](42c0c60) Thanks [@benchristel](https://github.com/benchristel)! - Internal: replace some brittle SVG snapshot tests with less brittle visual snapshot tests


-   [#1271](#1271) [`55039a430`](55039a4) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Bugfix: Arrowhead Rotation Was Incorrect on Some Graphs


-   [#1295](#1295) [`f6be03dd8`](f6be03d) Thanks [@benchristel](https://github.com/benchristel)! - Fix a bug where the arrow at the end of a line or ray would sometimes point to the origin and not the edge of the graph


-   [#1294](#1294) [`fba227fe8`](fba227f) Thanks [@benchristel](https://github.com/benchristel)! - Fix a bug where axis tick labels on Mafs interactive graphs could be selected and interfere with drag interactions


-   [#1255](#1255) [`dc0adedeb`](dc0aded) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Ensure that axis lines and arrowheads have a 2px thickness in Interactive Graphs


-   [#1264](#1264) [`d70fab6a7`](d70fab6) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Show Radio Widget Images on New Line


-   [#1285](#1285) [`5b52a1569`](5b52a15) Thanks [@benchristel](https://github.com/benchristel)! - Internal: refactor the code for initializing linear graph states

## @khanacademy/perseus-editor@6.5.0

### Minor Changes

-   [#1277](#1277) [`f8539c880`](f8539c8) Thanks [@nishasy](https://github.com/nishasy)! - Shows error in the editor if locked line has length 0


-   [#1284](#1284) [`8534a9f01`](8534a9f) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Add ToggleableCaret component and use in TexErrorView

### Patch Changes

-   [#1287](#1287) [`d9b51dcc0`](d9b51dc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Interactive Graph Editor: Make the common graph settings a collapsable panel


-   [#1278](#1278) [`fffd130db`](fffd130) Thanks [@nishasy](https://github.com/nishasy)! - Nit: put the line kind dropdown options in alphabetical order


-   [#1280](#1280) [`5b1e04abc`](5b1e04a) Thanks [@nishasy](https://github.com/nishasy)! - Fix the bug where the first added locked figure settings would be collapsed when it's supposed to be expanded on add

-   Updated dependencies \[[`e14a003be`](e14a003), [`42c0c607f`](42c0c60), [`55039a430`](55039a4), [`f6be03dd8`](f6be03d), [`fba227fe8`](fba227f), [`dc0adedeb`](dc0aded), [`a0dfc66cc`](a0dfc66), [`d70fab6a7`](d70fab6), [`5b52a1569`](5b52a15)]:
    -   @khanacademy/perseus@22.6.0

## @khanacademy/perseus-dev-ui@1.5.8

### Patch Changes

-   [#1291](#1291) [`cceca19c7`](cceca19) Thanks [@benchristel](https://github.com/benchristel)! - Update dependency versions


-   [#1290](#1290) [`d41feb9be`](d41feb9) Thanks [@benchristel](https://github.com/benchristel)! - Internal: upgrade to @khanacademy/wonder-blocks-timing@5 in dev UI

Author: khan-actions-bot

Reviewers: benchristel

Required Reviewers:

Approved By: benchristel

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

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