Fix: Open unlimited polygon does not restrict crossing sides. #1896
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (5831675) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1896If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1896 |
|
Size Change: +134 B (+0.01%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
catandthemachines
requested review from
benchristel,
nishasy and
anakaren-rojas
| @@ -440,7 +446,7 @@ const renderPolygonGraphControls = (props: { | |||
| width: "100%", | |||
| marginLeft: "20px", | |||
| }} | |||
| tabIndex={0} | |||
| tabIndex={coords.length < 3 ? -1 : 0} | |||
There was a problem hiding this comment.
Can you add a comment explaining why the tab index depends on the coords length being less than 3?
| @@ -483,8 +483,13 @@ function doMovePoint( | |||
| newValue: newValue, | |||
| }); | |||
|
|
|||
| // Reject the move if it would cause the sides of the polygon to cross | |||
| if (polygonSidesIntersect(newCoords)) { | |||
| // Boolean value to tract whether we can let the polygon sides interact. | |||
There was a problem hiding this comment.
| // Boolean value to tract whether we can let the polygon sides interact. | |
| // Boolean value to track whether we can let the polygon sides interact. |
| @@ -301,6 +304,9 @@ export const MafsGraph = (props: MafsGraphProps) => { | |||
| {interactionPrompt && ( | |||
| <View | |||
| style={{ | |||
| display: interactionPrompt | |||
| ? undefined | |||
| : "hidden", | |||
There was a problem hiding this comment.
Nit: Can you add a brief note in your PR description saying that this change is also in your PR even though it's not the main focus? It's helpful for me to follow PR expectations that way, and I think it's nice to document for posterity.
There was a problem hiding this comment.
What happens when closing the polygon is what causes the sides to cross? Can you then untangle the points using the keyboard, or are they stuck?
I think we might need some logic like "if the polygon's sides currently cross, don't reject a move that results in the sides still crossing."
There was a problem hiding this comment.
The changes to this file seem unrelated to the rest of the PR. Are they supposed to be included?
Summary:
For keyboard users if they're drowing a polygon and it places a point at 0,0. There are cases where it could cross the sides of the polygon they are actively creating and cause the polygon to break. Mouse uses can get around this a bit. But it's not idea.
This fix checks whether the polygon type allows intersecting sides and disables the rejection logic.
Issue: LEMS-2618
Test plan:
Go to /?path=/story/perseus-widgets-interactive-graph--unlimited-polygon-with-mafs in Storybook.
Use your keyboard to add points.
Move a few points in a way that a new point would cause the sides to intersect.
Add a new point and make sure you can still move points on the polygon.