feat: add showAngle field to LockedPolygonPointType#3730
Conversation
…edPolygonPointType`
|
Size Change: +156 B (+0.03%) Total Size: 507 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (abed06b) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3730If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3730If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3730 |
| {points.map((point, i) => { | ||
| const previous = points.at(i - 1); | ||
| const next = points.at((i + 1) % points.length); | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions |
There was a problem hiding this comment.
This is mirroring the pattern in packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx.
There was a problem hiding this comment.
We could probably improve the undefined check to avoid the strict boolean expressions :) I think the old logic predated the inclusion of the linter rule
…ld to `LockedPolygonPointType`. This option controls the display of an angle label per-vertex. It defaults to false.
jeremywiebe
left a comment
There was a problem hiding this comment.
In the Chromatic version of Storybook, the editor layout looks pretty janky.
Could you double-check that before landing, please? If you need design help, please ask Caitlyn about the layout.
| <Button | ||
| kind="tertiary" | ||
| startIcon={plusCircle} | ||
| onClick={() => { | ||
| props.onChangeProps({ | ||
| points: [...points, {coord: [0, 0]}], | ||
| points: [ | ||
| ...points, | ||
| {coord: [0, 0], showAngle: false}, | ||
| ], | ||
| }); | ||
| }} | ||
| > |
There was a problem hiding this comment.
I know this button predates your change, but would you mind adding an aria-label to this button (it took me a bit to decipher that it was the "add point" button... right? 😬 ).
| showAngles={point.showAngle} | ||
| // Locked polygons don't snap; "grid" is chosen as a | ||
| // default to preserve decimal angle labels. | ||
| snapTo="grid" |
There was a problem hiding this comment.
thought: This is an unfortunate instance where we propagate a value from higher up and its meaning is completely different inside this component. Ideally, this prop would be named something like roundLabelAngleToInteger or something.
No change required.
There was a problem hiding this comment.
Yeah this does seem like a potential ticket I could throw in our LEMS backlog, since it's a little outside of this PR
|
|
||
| const parseLockedPolygonPointType = object({ | ||
| coord: pairOfNumbers, | ||
| showAngle: defaulted(boolean, () => false), |
There was a problem hiding this comment.
I think we want to add a regression test item now that we have a new value here.
SonicScrewdriver
left a comment
There was a problem hiding this comment.
Great work! Overall this is nearly there I think we just need to polish up the design a little!
| showAngle: newValue, | ||
| }; | ||
| props.onChangeProps({points: newPoints}); | ||
| }} |
There was a problem hiding this comment.
I do think this needs some tweaking in the editor UI, as the option is clipping and doesn't quite match the rest of the styles. Unfortunately we're quite constrained in the width of the editor settings area right now. I'm happy to pair if you want to discuss how we could approach the layout!
I did notice that the start coordinates handle the extra input for a point by adding a new surrounding box:
| {points.map((point, i) => { | ||
| const previous = points.at(i - 1); | ||
| const next = points.at((i + 1) % points.length); | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions |
There was a problem hiding this comment.
We could probably improve the undefined check to avoid the strict boolean expressions :) I think the old logic predated the inclusion of the linter rule
| showAngles={point.showAngle} | ||
| // Locked polygons don't snap; "grid" is chosen as a | ||
| // default to preserve decimal angle labels. | ||
| snapTo="grid" |
There was a problem hiding this comment.
Yeah this does seem like a potential ticket I could throw in our LEMS backlog, since it's a little outside of this PR
| {coord: [-6.5, -4.5], showAngle: false}, | ||
| ], | ||
| weight, | ||
| color: "pink", |
There was a problem hiding this comment.
It'd be great if you could also add a story to this storybook regression test where showAngle is on for a locked polygon. These stories get snapshot and checked by Chromatic/Storybook so that we're notified on a PR if anything changes visually, which has been quite helpful for our Interactive Graph Widget.
Summary
This is a follow-up to #3705.
showAngleto locked polygon points, defaulting to false for parsed legacy data and newly createdpoints.
per vertex.
Test plan
pnpm test