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

Interactive Graph Editor: Make shared graph settings collapsable to save space #1287

Merged
merged 8 commits into from
May 21, 2024

Conversation

jeremywiebe
Copy link
Collaborator

@jeremywiebe jeremywiebe commented May 16, 2024

Summary:

The shared graph settings (like range, snap step, grid step, etc). take up a bunch of vertical space in the graph editor. Now that we're also adding locked figures, the editor is growing even taller and unwieldy.

This PR adds a small heading above the common graph settings and enables "tap-to-toggle" on that header to expand/collapse the area.

Screen.Recording.2024-05-16.at.3.05.58.PM.mov

Issue: "none"

Test plan:

@jeremywiebe jeremywiebe self-assigned this May 16, 2024
Copy link
Contributor

github-actions bot commented May 16, 2024

Size Change: +313 B (+0.04%)

Total Size: 839 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 268 kB +355 B (+0.13%)
packages/perseus/dist/es/index.js 403 kB -42 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-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 16, 2024

Codecov Report

Attention: Patch coverage is 75.34247% with 72 lines in your changes are missing coverage. Please review.

Project coverage is 70.15%. Comparing base (9d27c76) to head (9e90d37).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1287      +/-   ##
==========================================
+ Coverage   68.88%   70.15%   +1.27%     
==========================================
  Files         477      481       +4     
  Lines      101842   102000     +158     
  Branches     5121    10309    +5188     
==========================================
+ Hits        70151    71558    +1407     
+ Misses      31576    30442    -1134     
+ Partials      115        0     -115     

Impacted file tree graph

Files Coverage Δ
...itor/src/components/interactive-graph-settings.tsx 89.63% <75.34%> (-2.12%) ⬇️

... and 131 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 9d27c76...9e90d37. Read the comment docs.

Base automatically changed from jer/toggleable-caret to main May 16, 2024 22:17
@jeremywiebe jeremywiebe marked this pull request as ready for review May 17, 2024 17:51
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented May 17, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/grumpy-laws-arrive.md, packages/perseus-editor/src/components/interactive-graph-settings.tsx, 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.

@jeremywiebe jeremywiebe requested review from nishasy, mark-fitzgerald and a team May 17, 2024 17:52
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 (9e90d37) and published it to npm. You
can install it using the tag PR1287.

Example:

yarn add @khanacademy/perseus@PR1287

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

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

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.

I left a couple comments inline, but once those are addressed I think this is good to go!

.changeset/tidy-otters-lick.md Outdated Show resolved Hide resolved
padding: spacing.xSmall_8,
marginTop: spacing.small_12,
marginLeft: -10,
marginRight: -10,
Copy link
Member

Choose a reason for hiding this comment

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

Could we use spacing constants here instead of the magic -10?

FEI is talking about switching to rems for all measurement, for accessibility. It would be great to reduce our dependency on pixels if we can.

Also, minor: marginInline is the i18n-ready version of marginLeft + marginRight. https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched to marginInline, thanks.

For now, this must be exactly -10 because it negates the @editorPadding Less variable from perseus-editor.less.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a comment. But I'll reproduce it here. The value is the inverse of a Less variable @editorPadding. If we moved the value to be a CSS variable, we could use it in both the Less file var(--editor-padding) as well as in this Aphrodite style (marginInline: calc(-1 * --editor-padding).

I haven't done that in this PR though as I think we'd want to talk about if that's a way we want to move forward before I start a new pattern.

@jeremywiebe jeremywiebe merged commit d9b51dc into main May 21, 2024
11 of 13 checks passed
@jeremywiebe jeremywiebe deleted the jer/locked-figures-heading branch May 21, 2024 21:19
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
3 participants