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

Remove 'Show ruler' option from interactive graph editor #1456

Merged
merged 5 commits into from
Jul 29, 2024

Conversation

benchristel
Copy link
Member

@benchristel benchristel commented Jul 29, 2024

The ruler is not used in published content. We haven't implemented it for
the new Mafs version of the InteractiveGraph widget, so there's no point
having a checkbox for it in the editor.

Issue: LEMS-2096

Test plan:

View an interactive graph exercise in the exercise editor:
/devadmin/content/exercises/linear-graph-exercise/x3239cabc044aa28b/x325a92c13bc37711

There should be no option to show the ruler in the graph settings.

Screen Shot 2024-07-29 at 2 35 35 PM

@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Jul 29, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/breezy-mirrors-suffer.md, packages/perseus/src/perseus-types.ts, packages/perseus/src/util/test-utils.ts, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus-editor/src/__stories__/editor.stories.tsx, packages/perseus-editor/src/components/interactive-graph-settings.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor.tsx, packages/perseus/src/widgets/__testdata__/interactive-graph-random.testdata.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.ts, packages/perseus-editor/src/components/__tests__/interactive-graph-settings.test.tsx

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

Copy link
Contributor

github-actions bot commented Jul 29, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1456

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

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

Copy link
Contributor

github-actions bot commented Jul 29, 2024

Size Change: -593 B (-0.07%)

Total Size: 849 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 270 kB -523 B (-0.19%)
packages/perseus/dist/es/index.js 411 kB -70 B (-0.02%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.2 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.6 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/perseus/dist/es/strings.js 3.21 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.61%. Comparing base (7e71f8e) to head (5bb843e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1456      +/-   ##
==========================================
+ Coverage   69.85%   70.61%   +0.76%     
==========================================
  Files         505      508       +3     
  Lines      105003   104874     -129     
  Branches     7540    11398    +3858     
==========================================
+ Hits        73347    74056     +709     
+ Misses      31540    30818     -722     
+ Partials      116        0     -116     

Impacted file tree graph

Files Coverage Δ
...itor/src/components/interactive-graph-settings.tsx 98.89% <100.00%> (+0.08%) ⬆️
packages/perseus/src/util/test-utils.ts 100.00% <ø> (ø)
packages/perseus/src/widgets/interactive-graph.tsx 45.01% <ø> (-0.21%) ⬇️
...ctive-graphs/interactive-graph-question-builder.ts 100.00% <ø> (ø)
...us-editor/src/widgets/interactive-graph-editor.tsx 91.25% <0.00%> (+0.12%) ⬆️

... and 130 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 7e71f8e...5bb843e. Read the comment docs.

Copy link
Contributor

@nicolecomputer nicolecomputer left a comment

Choose a reason for hiding this comment

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

This looks great 🚢

@@ -64,10 +64,7 @@ class InteractiveGraphQuestionBuilder {
labels: this.labels,
markings: this.markings,
range: [this.xRange, this.yRange],
rulerLabel: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

This builder pattern is lovely because we don't have to go find out every place that we have test data and remove these. 💪

Copy link
Contributor

@mark-fitzgerald mark-fitzgerald left a comment

Choose a reason for hiding this comment

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

Code cleansing is so refreshing!

@benchristel benchristel merged commit b868801 into main Jul 29, 2024
19 checks passed
@benchristel benchristel deleted the benc/remove-ruler branch July 29, 2024 21:44
benchristel pushed a commit that referenced this pull request Jul 29, 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@27.0.0

### Major Changes

-   [#1456](#1456) [`b868801fa`](b868801) Thanks [@benchristel](https://github.com/benchristel)! - Remove unused "Show ruler" option from the interactive graph editor. The
    new Mafs version of the interactive graph does not implement the ruler,
    and we have no plans to implement it, since it can't be made accessible
    and isn't used in Khan Academy's existing content.


-   [#1450](#1450) [`2216ad012`](2216ad0) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove Unit aka UnitInput widget

### Minor Changes

-   [#1451](#1451) [`9bc4812fc`](9bc4812) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Upgrade Mafs Dependency to v0.19.0 for Domain Restricted Functions


-   [#1441](#1441) [`9bc264ce1`](9bc264c) Thanks [@Myranae](https://github.com/Myranae)! - Turn off interactivity when Interactive Graph in hint mode


-   [#1437](#1437) [`7a448e77c`](7a448e7) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon to be filled on hover


-   [#1422](#1422) [`c386515ad`](c386515) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Segment and Linear System graph start coords UI

### Patch Changes

-   [#1448](#1448) [`84675574c`](8467557) Thanks [@nishasy](https://github.com/nishasy)! - Cleaned up the `startCoords` code in the stateful mafs graph useEffect


-   [#1444](#1444) [`130ab9446`](130ab94) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Throttles point-moving performance in polygon


-   [#1445](#1445) [`bb1ac584b`](bb1ac58) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - useDraggable - fix to ignore keyup events (we don't want to treat keyup events as an intent to move - we have keydown for that)

-   Updated dependencies \[[`7e71f8e8a`](7e71f8e)]:
    -   @khanacademy/math-input@20.0.2

## @khanacademy/perseus-editor@11.0.0

### Major Changes

-   [#1456](#1456) [`b868801fa`](b868801) Thanks [@benchristel](https://github.com/benchristel)! - Remove unused "Show ruler" option from the interactive graph editor. The
    new Mafs version of the interactive graph does not implement the ruler,
    and we have no plans to implement it, since it can't be made accessible
    and isn't used in Khan Academy's existing content.


-   [#1450](#1450) [`2216ad012`](2216ad0) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove Unit aka UnitInput widget

### Minor Changes

-   [#1422](#1422) [`c386515ad`](c386515) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Segment and Linear System graph start coords UI

### Patch Changes

-   [#1446](#1446) [`4985d2d4c`](4985d2d) Thanks [@nishasy](https://github.com/nishasy)! - Rename StartCoordSettings to StartCoordsSettings


-   [#1448](#1448) [`84675574c`](8467557) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Refactor and clean up start coords UI implementation

-   Updated dependencies \[[`b868801fa`](b868801), [`84675574c`](8467557), [`7e71f8e8a`](7e71f8e), [`9bc4812fc`](9bc4812), [`130ab9446`](130ab94), [`9bc264ce1`](9bc264c), [`bb1ac584b`](bb1ac58), [`2216ad012`](2216ad0), [`7a448e77c`](7a448e7), [`c386515ad`](c386515)]:
    -   @khanacademy/perseus@27.0.0
    -   @khanacademy/math-input@20.0.2

## @khanacademy/math-input@20.0.2

### Patch Changes

-   [#1454](#1454) [`7e71f8e8a`](7e71f8e) Thanks [@Myranae](https://github.com/Myranae)! - Update mathjax-renderer version

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

### Patch Changes

-   Updated dependencies \[[`7e71f8e8a`](7e71f8e)]:
    -   @khanacademy/math-input@20.0.2

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), ✅ Cypress (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), ✅ gerald

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