Circle Graph foundation #1237
Circle Graph foundation #1237
Conversation
handeyeco
changed the title
|
Size Change: +1.11 kB (0%) Total Size: 834 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1237 +/- ##
==========================================
+ Coverage 68.64% 69.85% +1.20%
==========================================
Files 470 475 +5
Lines 100733 101030 +297
Branches 7146 10190 +3044
==========================================
+ Hits 69148 70571 +1423
+ Misses 31406 30459 -947
+ Partials 179 0 -179
... and 140 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
This looks like a good start! Was there something in particular you wanted feedback on?
I noticed that it's possible to drag either control point outside the graph bounds. We should have constraints that keep both points visible.
A minor UX thing: it might be nice if the cursor changed to a left-right arrow when you hover over the radius point. Otherwise, it's slightly surprising that that point can only move right and left.
| switch (state.type) { | ||
| case "circle": { | ||
| const nextRadiusPoint: vec.Vector2 = [ | ||
| // The +0 is to convert -0 to +0 |
There was a problem hiding this comment.
ugh, floating point.
no action needed :P
| @@ -71,6 +71,7 @@ export interface CircleGraphState extends InteractiveGraphStateCommon { | |||
| type: "circle"; | |||
| center: Coord; | |||
| radius: number; | |||
| radiusPoint: Coord; | |||
There was a problem hiding this comment.
Does the radiusPoint have to be part of the state? Since it seems the radiusPoint can only be moved left and right, it could probably be computed from the radius. Either that, or the radius could be computed from the center and the radiusPoint.
If both radius and radiusPoint exist on the state, then we have multiple sources of truth for what the radius should be, and we have to take extra care to keep them in sync.
There was a problem hiding this comment.
Originally I was just using radiusPoint. I added back radius when I looked at grading, but I think I could remove it again.
There was a problem hiding this comment.
Okay, I remember what it was @benchristel: I wanted to store radiusPoint because I figured radius could just be computed from that and it'd give me some flexibility around positioning the handle.
However the way things worked was that MafsGraph was just blindingly passing its state up to InteractiveGraph. InteractiveGraph merges the object from the onChange callback into its state and without radius the editor doesn't work as expected.
This made me wonder: do we have to be bound to the structure of state in the legacy interactive graph? So I updated my code to include mafStateToInteractiveGraph, the idea being that we could store whatever we want, however we want in the new code as long as we transform it to be compatible with the legacy code. I expect this could be controversial, but I also imagine it could be useful as we implement the weirder charts and try to add a11y improvements.
Obviously I could also just add radius back to the store.
There was a problem hiding this comment.
FWIW, I like the approach of keeping only the state we need in our useReducer store, and transforming it back to the legacy format only when we have to.
Currently, someone who's only looking at the useReducer state type has to wonder "why did they design it this way" and it's unsatisfying if the answer is "an accident of history."
packages/perseus/src/widgets/interactive-graphs/graphs/circle.tsx
Outdated
Show resolved
Hide resolved
Screen.Recording.2024-04-29.at.4.14.21.PM.movMade the constraints keep the center dot in the chart and did what I could to keep the radius handle in view. |
Cool! One thought, though: when the radius handle swaps over to the left side, the keyboard controls get reversed: left arrow now expands the circle, whereas normally it contracts it. This is likely to be confusing for screenreader and keyboard users. Note that with the current setup, I think this is the only way to get the point to go over to the left side if you're only moving things with the keyboard. I think we can simplify this. Let's start with the assumption that the entire circle must remain visible within the graph bounds. If we need to revisit that decision, we will. |
| return Math.sqrt( | ||
| Math.pow(edgeX - centerX, 2) + Math.pow(edgeY - centerY, 2), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Minor: I think this getRadius function could live with the definition of the circle graph state type, and take the entire graph state as its argument:
getRadius(circleGraph);...and then getRadius could live in interactive-graph-state.ts.
Our way of representing a circle, as a center point and a point on the edge, is pretty specific to the graph state format. Passing the entire graph state to getRadius would better encapsulate the structure of the state, and possibly let us change it more easily in the future.
(in general, the practice of passing entire state objects around opens up other possibilities, too, like caching the results of getter functions — see flipbook-model.ts for examples. I don't think caching is relevant here, though)
handeyeco
changed the title
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
@nishasy I added you specifically as a reviewer because I'm confused about why I needed to change the snapshots and I was hoping you might know. |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (6ffcacb) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1237If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1237 |
| @@ -45,6 +52,10 @@ export type Props = { | |||
| labels: InteractiveGraphProps["labels"]; | |||
| }; | |||
|
|
|||
| type MafsChange = { | |||
| graph: InteractiveGraphState; | |||
| }; | |||
There was a problem hiding this comment.
I was confused for a bit why this was called MafsChange. I wonder if we could inline this type, or maybe simplify things so we're only passing the InteractiveGraphState around, instead of an object that contains the state.
There was a problem hiding this comment.
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Outdated
Show resolved
Hide resolved
| const xJumpDist = (radX - constrainedCenter[0]) * 2; | ||
| const possibleNewX = newRadiusPoint[0] - xJumpDist; | ||
| if (possibleNewX >= xMin && possibleNewX <= xMax) { | ||
| newRadiusPoint[0] = possibleNewX; |
There was a problem hiding this comment.
This reassignment makes me a bit nervous. We're assuming that vec.add is always going to create a new object.
I'd prefer to make newRadiusPoint a let binding and reassign it:
newRadiusPoint = [possibleNewX, newRadiusPoint[1]];Alternatively, we could scope the creation of newRadiusPoint inside an IIFE, and then we don't need reassignments at all:
const newRadiusPoint = (() => {
const newRadiusPoint = vec.add(
state.radiusPoint,
vec.sub(constrainedCenter, state.center),
);
// Try to position the radius handle in a visible spot
// if it otherwise would be off the chart
// ex: if the handle is on the right and we move the center
// to the rightmost position, move the handle to the left
const [radX, radY] = newRadiusPoint;
if (radX < xMin || radX > xMax) {
const xJumpDist = (radX - constrainedCenter[0]) * 2;
const possibleNewX = radX - xJumpDist;
if (possibleNewX >= xMin && possibleNewX <= xMax) {
return [possibleNewX, radY];
}
}
return newRadiusPoint;
})();There was a problem hiding this comment.
I added a spread operator to at least make sure it's not the same array object:
const newRadiusPoint: vec.Vector2 = [
...vec.add(
state.radiusPoint,
vec.sub(constrainedCenter, state.center),
),
];
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Outdated
Show resolved
Hide resolved
| const [edgeX, edgeY] = graph.radiusPoint; | ||
| return Math.sqrt( | ||
| Math.pow(edgeX - centerX, 2) + Math.pow(edgeY - centerY, 2), | ||
| ); |
There was a problem hiding this comment.
The Pythagorean theorem feels like overkill here, but I guess it's less likely to break in the future.
There was a problem hiding this comment.
I personally like it for being future-proof.
| @@ -3302,15 +3302,15 @@ exports[`snapshots should render correctly with locked lines 1`] = ` | |||
| > | |||
| <pattern | |||
| height="20" | |||
| id="cartesian-9" | |||
| id="cartesian-31" | |||
There was a problem hiding this comment.
I'm not 100% certain, but I think this snapshot update has to do with how Mafs does the ID for the Cartesian plane. It uses an incrementer as a hack for unique IDs. I really don't know why there are 22 more cartesian planes here than there were before though??
I also don't understand why the snapshot even previously only had cartesian-8 and cartesian-9. Not sure where 1-7 went 🤔
I wonder if @jeremywiebe would know?
There was a problem hiding this comment.
I suspect that tests that involve the <Cartesian /> component "burn" ids even if they don't involve capturing a snapshot. As long as the tests (including snapshot tests) run in the same order (which it seems they do) we should be ok with re-numbering these ids.
It'd be nice to have something like useUniqueId() from WB, but I don't think it's reasonable to introduce that into Mafs.
There was a problem hiding this comment.
I think we need to find a fix for this, I keep having to regenerate snapshots.
Maybe we need to re-init Mafs before each test?
|
Everything's passing except some Changeset weirdness. I think it's safe to move ahead. |
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.4.0 ### Minor Changes - [#1229](#1229) [`3c1e398d5`](3c1e398) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Show Arrowheads on Locked Lines in Interactive Graphs - [#1237](#1237) [`54689a18f`](54689a1) Thanks [@handeyeco](https://github.com/handeyeco)! - Rough out new Circle Graph behind a feature flag ### Patch Changes - [#1222](#1222) [`44cf7348c`](44cf734) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Fix @phosphor-icon paths in `explanation` widget - [#1243](#1243) [`ee89a1b01`](ee89a1b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Fix dash style for locked lines when kind is 'ray' ## @khanacademy/perseus-editor@6.2.1 ### Patch Changes - [#1237](#1237) [`54689a18f`](54689a1) Thanks [@handeyeco](https://github.com/handeyeco)! - Rough out new Circle Graph behind a feature flag - [#1246](#1246) [`d66b79e44`](d66b79e) Thanks [@nishasy](https://github.com/nishasy)! - Change locked figures' initial color to grayH (previusly green) - [#1242](#1242) [`7d172698e`](7d17269) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Adds a warning above the protractor and ruler checkboxes in interactive-graph settings - [#1245](#1245) [`45a6647cf`](45a6647) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Fix location of DeviceFramer and ViewportResizer in Storybook - Updated dependencies \[[`44cf7348c`](44cf734), [`3c1e398d5`](3c1e398), [`ee89a1b01`](ee89a1b), [`54689a18f`](54689a1)]: - @khanacademy/perseus@22.4.0 ## @khanacademy/perseus-dev-ui@1.5.5 ### Patch Changes - [#1222](#1222) [`44cf7348c`](44cf734) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - ✨ Display image background info in Dev UI
Summary:
Roughing out the Mafs Circle Graph.
Learner Experience
Screen.Recording.2024-04-29.at.1.36.21.PM.mov
Editor Experience
Screen.Recording.2024-04-29.at.1.37.00.PM.mov
Issue: LEMS-1828
Test plan: