[Interactive Graph] Update protractor to new SVG #2263
Merged
+17
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s going in both directions
The old protractor looked awful. We got a request to use a protractor with two rows of numbers going in opposite directions (previously there was only one direction). - Update the protractor to a new SVG with numbers going both ways Note: The numbers are kind of hard to read over the axes. I tried to add white backgrounds to the numbers, but it's unfortunately not as easy as it looks. Since this is not a regression, I left it as is. Note 2: There is a noticable performance hit with this implementation. If someone has ideas for how to improve the performance, please let me know! Issue: https://khanacademy.atlassian.net/browse/LEMS-2863 Test plan: Storybook - https://khan.github.io/perseus/?path=/story/perseus-widgets-interactive-graph-visual-regression-tests--mafs-with-protractor
|
Size Change: +1 B (0%) Total Size: 872 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (67965da) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2263If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2263 |
nishasy
requested review from
SonicScrewdriver,
catandthemachines and
a team
jeremywiebe
approved these changes
Feb 25, 2025
There was a problem hiding this comment.
Nice! I had two thoughts (neither of which are blocking):
- This increases the bundle size by about 45kb. Previously we loaded the protractor image remotely which meant that the image size didn't increase the main Perseus bundle size. Perhaps you could touch base with FEI to see their thoughts on something like this? I think it makes sense to embed the SVG ... and perhaps the gzip compression we apply when serving the JS bundles will negate the increase.
- Should we change the mouse cursor to the move handle instead the default arrow? I notice for other movables in the graph we do that, but not when moving the protractor. This seems like a new change, but it felt "off" that the mouse cursor didn't change to show I could drag the protractor (but it did for the double-ended arrow that allows me to rotate the protractor)
Also, for posterity, here's the Chromatic publish of Storybook with the new Protractor SVG: https://650db21c3f5d1b2f13c02952-bhaakpommq.chromatic.com/?path=/story/perseus-widgets-interactive-graph-visual-regression-tests--mafs-with-protractor
Comment on lines
243
to
247
| stroke: #808080; | ||
| } | ||
|
|
||
| .MafsView .interactive-graph-protractor .protractor-number { | ||
| fill: #21242c; |
There was a problem hiding this comment.
Are these two colours represented in CSS variables? It's likely not worth the effort to create them if they don't exist given that the Protractor isn't a tool we want to encourage new usages of, though.
jeremywiebe
approved these changes
Feb 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The old protractor looked awful. We got a request to use a protractor
with two rows of numbers going in opposite directions (previously
there was only one direction).
Note: The numbers are kind of hard to read over the axes. I tried to add
white backgrounds to the numbers, but it's unfortunately not as easy as
it looks. Since this is not a regression, I left it as is.
Issue: https://khanacademy.atlassian.net/browse/LEMS-2863
Test plan:
Storybook