-
Notifications
You must be signed in to change notification settings - Fork 350
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] Open and Closing logic for unlimited polygon. #1852
Merged
Merged
Changes from 27 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
aa64c95
Got a basic open polygon experience working.
catandthemachines e5876f0
Adding more comments.
catandthemachines e68261e
Simplifying logic.
catandthemachines bca7e2a
Adding attempts to simply logic between the two components
catandthemachines 184993d
Adding strings and buttons to add and removing points.
catandthemachines 2ee6162
Trying to improve the point graph
catandthemachines f24b118
Adding disabled funtionality.
catandthemachines 55b4415
Adding unit testing.
catandthemachines 7731db1
Adding new tests for opening and closing polygon.
catandthemachines e6c447e
Adding more tests.
catandthemachines 1ba3482
Adding logic to close shape when clicking on the first point.
catandthemachines f0418e6
Updating description.
catandthemachines 5aacf23
Adding additional key event.
catandthemachines fc6c767
Adding for posterity.
catandthemachines c95b58b
Merge branch 'main' into catjohnson/open-unlimited-poly
catandthemachines b0d53b8
Added comments based on chat w/ Caitlyn.
catandthemachines a29f8b8
Merge branch 'main' into catjohnson/open-unlimited-poly
catandthemachines 82ead83
Adding work to re-align buttons. Still more to add here.
catandthemachines 78e6542
Merge branch 'main' into catjohnson/open-unlimited-poly
catandthemachines 48699f0
Adding in disabled logic.
catandthemachines 4d03b20
Add graph lines to dev example.
catandthemachines cd416e2
adding changeset.
catandthemachines df98397
Removing a few TODOs, and fixing a few tests.
catandthemachines f5a2116
Adding more robust testing. <3
catandthemachines 8f13391
Cleaning up polygon file.
catandthemachines 44bcf54
Removing unneeded comment.
catandthemachines 186b768
Adding storybook example for unlimited polygon with mafs. <3
catandthemachines ff76028
updating bulk tests.
catandthemachines 762cc03
remove unnecessary props.
catandthemachines 8e860f5
Merge branch 'main' into catjohnson/open-unlimited-poly
catandthemachines 6c4a3b1
Moving new line to proper location.
catandthemachines 95470bb
Adding property to test.
catandthemachines d0842b3
removing comment.
catandthemachines File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/perseus": patch | ||
--- | ||
|
||
Adding open and closing behavior to unlimited polygon graph type. |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import {Polygon, vec} from "mafs"; | ||
import {Polygon, Polyline, vec} from "mafs"; | ||
import * as React from "react"; | ||
|
||
import {snap} from "../math"; | ||
|
@@ -169,32 +169,14 @@ const LimitedPolygonGraph = (props: Props) => { | |
); | ||
}; | ||
|
||
// TODO(catjohnson): reduce redundancy between LimitedPolygonGraph and UnlimitedPolygonGraph | ||
// both components are vary similar, however more implementation is needed to be added before | ||
// it is clear what can and can't be shared between components. | ||
const UnlimitedPolygonGraph = (props: Props) => { | ||
const [hovered, setHovered] = React.useState(false); | ||
// This is more so required for the re-rendering that occurs when state | ||
// updates; specifically with regard to line weighting and polygon focus. | ||
const [focusVisible, setFocusVisible] = React.useState(false); | ||
|
||
const {dispatch} = props; | ||
const { | ||
coords, | ||
showAngles, | ||
showSides, | ||
range, | ||
snapStep, | ||
snapTo = "grid", | ||
} = props.graphState; | ||
const {coords, closedPolygon} = props.graphState; | ||
|
||
const graphConfig = useGraphConfig(); | ||
|
||
// TODO(catjohnson): Explore abstracting this code as it is similar to point.tsx | ||
// and hopefully we can cut down ont the unlimited graph redundancy. | ||
const { | ||
range: [x, y], | ||
disableKeyboardInteraction, | ||
graphDimensionsInPixels, | ||
} = graphConfig; | ||
|
||
|
@@ -207,160 +189,84 @@ const UnlimitedPolygonGraph = (props: Props) => { | |
// TODO(benchristel): can the default set of points be removed here? I don't | ||
// think coords can be null. | ||
const points = coords ?? [[0, 0]]; | ||
const polygonRef = React.useRef<SVGPolygonElement>(null); | ||
const dragReferencePoint = points[0]; | ||
const constrain = ["angles", "sides"].includes(snapTo) | ||
? (p) => p | ||
: (p) => snap(snapStep, p); | ||
const {dragging} = useDraggable({ | ||
gestureTarget: polygonRef, | ||
point: dragReferencePoint, | ||
onMove: (newPoint) => { | ||
const delta = vec.sub(newPoint, dragReferencePoint); | ||
dispatch(actions.polygon.moveAll(delta)); | ||
}, | ||
constrainKeyboardMovement: constrain, | ||
}); | ||
|
||
const lines = getLines(points); | ||
React.useEffect(() => { | ||
const focusedIndex = props.graphState.focusedPointIndex; | ||
if (focusedIndex != null) { | ||
pointRef.current[focusedIndex]?.focus(); | ||
} | ||
}, [props.graphState.focusedPointIndex, pointRef]); | ||
|
||
return ( | ||
<> | ||
{/* This rect is here to grab clicks so that new points can be added */} | ||
{/* It's important because it stops mouse events from propogating | ||
if (closedPolygon) { | ||
const closedPolygonProps = {...props, numSides: coords.length}; | ||
return <LimitedPolygonGraph {...closedPolygonProps} />; | ||
} else { | ||
return ( | ||
<> | ||
{/* This rect is here to grab clicks so that new points can be added */} | ||
{/* It's important because it stops mouse events from propogating | ||
when dragging a points around */} | ||
<rect | ||
style={{ | ||
fill: "rgba(0,0,0,0)", | ||
cursor: "crosshair", | ||
}} | ||
width={widthPx} | ||
height={heightPx} | ||
x={left} | ||
y={top} | ||
onClick={(event) => { | ||
const elementRect = | ||
event.currentTarget.getBoundingClientRect(); | ||
<rect | ||
style={{ | ||
fill: "rgba(0,0,0,0)", | ||
cursor: "crosshair", | ||
}} | ||
width={widthPx} | ||
height={heightPx} | ||
x={left} | ||
y={top} | ||
onClick={(event) => { | ||
const elementRect = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can this be moved into a handler function? function handleOnClick(event) {
const elementRect =
event.currentTarget.getBoundingClientRect();
const x = event.clientX - elementRect.x;
const y = event.clientY - elementRect.y;
const graphCoordinates = pixelsToVectors(
[[x, y]],
graphConfig,
);
dispatch(actions.polygon.addPoint(graphCoordinates[0]));
} then <rect
style={{
fill: "rgba(0,0,0,0)",
cursor: "crosshair",
}}
width={widthPx}
height={heightPx}
x={left}
y={top}
onClick={handleOnClick}
/> |
||
event.currentTarget.getBoundingClientRect(); | ||
|
||
const x = event.clientX - elementRect.x; | ||
const y = event.clientY - elementRect.y; | ||
const x = event.clientX - elementRect.x; | ||
const y = event.clientY - elementRect.y; | ||
|
||
const graphCoordinates = pixelsToVectors( | ||
[[x, y]], | ||
graphConfig, | ||
); | ||
dispatch(actions.polygon.addPoint(graphCoordinates[0])); | ||
}} | ||
/> | ||
{/** | ||
* TODO(catjohnson): Will need to conditionally render then once a full polygon is created | ||
* And handle when someone wants to remove the polygon connection. | ||
*/} | ||
<Polygon | ||
points={[...points]} | ||
color="var(--movable-line-stroke-color)" | ||
svgPolygonProps={{ | ||
strokeWidth: focusVisible | ||
? "var(--movable-line-stroke-weight-active)" | ||
: "var(--movable-line-stroke-weight)", | ||
style: {fill: "transparent"}, | ||
}} | ||
/> | ||
{props.graphState.coords.map((point, i) => { | ||
const pt1 = points.at(i - 1); | ||
const pt2 = points[(i + 1) % points.length]; | ||
if (!pt1 || !pt2) { | ||
return null; | ||
} | ||
return ( | ||
<PolygonAngle | ||
key={"angle-" + i} | ||
centerPoint={point} | ||
endPoints={[pt1, pt2]} | ||
range={range} | ||
polygonLines={lines} | ||
showAngles={!!showAngles} | ||
snapTo={snapTo} | ||
/> | ||
); | ||
})} | ||
{showSides && | ||
lines.map(([start, end], i) => { | ||
const [x, y] = vec.midpoint(start, end); | ||
const length = parseFloat( | ||
vec | ||
.dist(start, end) | ||
.toFixed(snapTo === "sides" ? 0 : 1), | ||
); | ||
return ( | ||
<TextLabel key={"side-" + i} x={x} y={y}> | ||
{!Number.isInteger(length) && "≈ "} | ||
{length} | ||
</TextLabel> | ||
); | ||
})} | ||
{/** | ||
* This transparent svg creates a nice big click/touch target, | ||
* since the polygon itself can be made smaller than the spec. | ||
*/} | ||
{/** | ||
* Will likely want to conditionally render then once a full polygon is created | ||
* And handle when someone wants to remove the polygon connection? | ||
*/} | ||
<Polygon | ||
points={[...points]} | ||
color="transparent" | ||
svgPolygonProps={{ | ||
ref: polygonRef, | ||
tabIndex: disableKeyboardInteraction ? -1 : 0, | ||
strokeWidth: TARGET_SIZE, | ||
style: { | ||
cursor: dragging ? "grabbing" : "grab", | ||
fill: hovered ? "var(--mafs-blue)" : "transparent", | ||
}, | ||
onMouseEnter: () => setHovered(true), | ||
onMouseLeave: () => setHovered(false), | ||
// Required to remove line weighting when user clicks away | ||
// from the focused polygon | ||
onKeyDownCapture: () => { | ||
setFocusVisible(hasFocusVisible(polygonRef.current)); | ||
}, | ||
// Required for lines to darken on focus | ||
onFocus: () => | ||
setFocusVisible(hasFocusVisible(polygonRef.current)), | ||
// Required for line weighting to update on blur. Without this, | ||
// the user has to hover over the shape for it to update | ||
onBlur: () => | ||
setFocusVisible(hasFocusVisible(polygonRef.current)), | ||
className: "movable-polygon", | ||
}} | ||
/> | ||
{props.graphState.coords.map((point, i) => ( | ||
<MovablePoint | ||
key={i} | ||
point={point} | ||
onMove={(destination) => | ||
dispatch(actions.pointGraph.movePoint(i, destination)) | ||
} | ||
ref={(ref) => { | ||
pointRef.current[i] = ref; | ||
}} | ||
onFocus={() => { | ||
dispatch(actions.pointGraph.focusPoint(i)); | ||
const graphCoordinates = pixelsToVectors( | ||
catandthemachines marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[[x, y]], | ||
graphConfig, | ||
); | ||
dispatch(actions.polygon.addPoint(graphCoordinates[0])); | ||
}} | ||
onClick={() => { | ||
dispatch(actions.pointGraph.clickPoint(i)); | ||
/> | ||
<Polyline | ||
points={[...points]} | ||
color="var(--movable-line-stroke-color)" | ||
svgPolylineProps={{ | ||
strokeWidth: "var(--movable-line-stroke-weight)", | ||
style: {fill: "transparent"}, | ||
}} | ||
/> | ||
))} | ||
</> | ||
); | ||
{props.graphState.coords.map((point, i) => ( | ||
<MovablePoint | ||
key={i} | ||
point={point} | ||
onMove={(destination) => | ||
dispatch(actions.polygon.movePoint(i, destination)) | ||
} | ||
ref={(ref) => { | ||
pointRef.current[i] = ref; | ||
}} | ||
onFocus={() => { | ||
dispatch(actions.polygon.focusPoint(i)); | ||
}} | ||
onClick={() => { | ||
// If the point being clicked is the first point and | ||
// there's enough points to form a polygon (3 or more) | ||
// Close the shape before setting focus. | ||
if ( | ||
i === 0 && | ||
props.graphState.coords.length >= 3 | ||
) { | ||
dispatch(actions.polygon.closePolygon()); | ||
} | ||
dispatch(actions.polygon.clickPoint(i)); | ||
}} | ||
/> | ||
))} | ||
</> | ||
); | ||
} | ||
}; | ||
|
||
function getLines(points: readonly vec.Vector2[]): CollinearTuple[] { | ||
|
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit: the else isn't needed here since you return in your if statement