Skip to content

Commit

Permalink
[LX] Add hairlines when point has focus (#2212)
Browse files Browse the repository at this point in the history
## Summary:
Right now, we have hairlines showing when a user moves a point using a mouse, but the hairlines
don't show up when using keyboard.

Adding hairlines when the point is focused in this PR.

Issue: https://khanacademy.atlassian.net/browse/LEMS-2308

## Test plan:
`yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx`

Storybook
- http://localhost:6006/?path=/story/perseuseditor-widgets-interactive-graph--interactive-graph-point
- Move the points around with mouse and confirm that that behavior has not changed - hairlines
  should still be present.
- Tab to the points with keyboard and confirm that the hairlines now show up.
- Switch the graph to "none" markings, and confirm that the hairlines no longer show up.

### Screenshots when point is focused

| Before | After |
| --- | --- |
| <img width="485" alt="Screenshot 2025-02-06 at 4 15 36 PM" src="https://github.com/user-attachments/assets/de3adee4-789f-4a5a-90d8-c03b861d3f5e" /> | <img width="487" alt="Screenshot 2025-02-06 at 4 15 43 PM" src="https://github.com/user-attachments/assets/87510b7c-7524-4a36-a740-b697ad36390c" /> |

Author: nishasy

Reviewers: SonicScrewdriver, mark-fitzgerald, catandthemachines

Required Reviewers:

Approved By: SonicScrewdriver

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️  [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️  [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️  [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ⏹️  [cancelled] Check builds for changes in size (ubuntu-latest, 20.x)

Pull Request URL: #2212
  • Loading branch information
nishasy authored Feb 13, 2025
1 parent ae29e2b commit 3ec6ec1
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/pretty-spies-tan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

[LX] Add hairlines when point has focus
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type Props = {
point: vec.Vector2;
color?: string | undefined;
dragging: boolean;
focused: boolean;
showFocusRing: boolean;
cursor?: CSSCursor | undefined;
onClick?: () => unknown;
Expand All @@ -34,6 +35,7 @@ export const MovablePointView = forwardRef(
point,
color = WBColor.blue,
dragging,
focused,
cursor,
showFocusRing,
onClick = () => {},
Expand All @@ -60,7 +62,7 @@ export const MovablePointView = forwardRef(
const [[_, horizontalStartY]] = useTransformVectorsToPixels([0, yMin]);
const [[__, horizontalEndY]] = useTransformVectorsToPixels([0, yMax]);

const showHairlines = dragging && markings !== "none";
const showHairlines = (dragging || focused) && markings !== "none";
const hairlines = (
<g>
<line
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,52 @@ describe("MovablePoint", () => {
expect(hairLines).toHaveLength(2);
});

it("Hairlines do NOT show when not dragging", () => {
it("Shows hairlines when focused via keyboard and 'markings' are NOT set to 'none'", async () => {
useGraphConfigMock.mockReturnValue(baseGraphConfigContext);
const {container} = render(
<Mafs width={200} height={200}>
<MovablePoint
point={[0, 0]}
sequenceNumber={1}
onMove={() => {}}
/>
,
</Mafs>,
);

// Tab to the graph first.
await userEvent.tab();
// Tab to the point to give it focus.
await userEvent.tab();

// eslint-disable-next-line testing-library/no-container,testing-library/no-node-access
const hairLines = container.querySelectorAll("svg line");
expect(hairLines).toHaveLength(2);
});

it("Shows hairlines when focused via click and 'markings' are NOT set to 'none'", async () => {
useGraphConfigMock.mockReturnValue(baseGraphConfigContext);
const {container} = render(
<Mafs width={200} height={200}>
<MovablePoint
ariaLabel="point-label"
point={[0, 0]}
sequenceNumber={1}
onMove={() => {}}
/>
,
</Mafs>,
);

const point = screen.getByLabelText("point-label");
await userEvent.click(point);

// eslint-disable-next-line testing-library/no-container,testing-library/no-node-access
const hairLines = container.querySelectorAll("svg line");
expect(hairLines).toHaveLength(2);
});

it("Hairlines do NOT show when not dragging and not focused", () => {
useGraphConfigMock.mockReturnValue(baseGraphConfigContext);
const {container} = render(
<Mafs width={200} height={200}>
Expand All @@ -201,7 +246,7 @@ describe("MovablePoint", () => {
expect(hairLines).toHaveLength(0);
});

it("Hairlines do NOT show when 'markings' are set to 'none'", () => {
it("Hairlines do NOT show when dragging and 'markings' are set to 'none'", () => {
const graphStateContext = {...baseGraphConfigContext};
graphStateContext.markings = "none";
useGraphConfigMock.mockReturnValue(graphStateContext);
Expand All @@ -221,6 +266,31 @@ describe("MovablePoint", () => {
const hairLines = container.querySelectorAll("svg line");
expect(hairLines).toHaveLength(0);
});

it("Hairlines do NOT show when focused and 'markings' are set to 'none'", async () => {
const graphStateContext = {...baseGraphConfigContext};
graphStateContext.markings = "none";
useGraphConfigMock.mockReturnValue(graphStateContext);
const {container} = render(
<Mafs width={200} height={200}>
<MovablePoint
point={[0, 0]}
sequenceNumber={1}
onMove={() => {}}
/>
,
</Mafs>,
);

// Tab to the graph first.
await userEvent.tab();
// Tab to the point to give it focus.
await userEvent.tab();

// eslint-disable-next-line testing-library/no-container,testing-library/no-node-access
const hairLines = container.querySelectorAll("svg line");
expect(hairLines).toHaveLength(0);
});
});

it("calls onFocus() when you tab to it", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ export function useControlPoint(params: Params): Return {
}}
point={point}
dragging={dragging}
focused={focused}
color={color}
ref={visiblePointRef}
showFocusRing={focused}
Expand Down

0 comments on commit 3ec6ec1

Please sign in to comment.