Remove unlimited points via keyboard #1570

nicolecomputer · 2024-08-28T17:39:12Z

Summary:

remove-point-keyboard.mov

Remove unlimited points via keyboard.

A point can be removed by hitting delete
After a point is deleted the graph is focused

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

Test plan:

Run the tests
Everything should pass

khan-actions-bot · 2024-08-28T17:39:29Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/strange-crews-dress.md, packages/perseus/src/widgets/__tests__/interactive-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/types.ts, packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx, packages/perseus/src/widgets/interactive-graphs/reducer/initialize-graph-state.ts, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-action.ts, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-08-28T17:42:08Z

Size Change: +406 B (+0.05%)

Total Size: 861 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	418 kB	+406 B (+0.1%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	277 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.36 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-08-28T18:11:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.76%. Comparing base (21a908e) to head (c3ebead).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1570      +/-   ##
==========================================
+ Coverage   70.34%   70.76%   +0.42%     
==========================================
  Files         521      544      +23     
  Lines      107316   111334    +4018     
  Branches     5513    11890    +6377     
==========================================
+ Hits        75489    78791    +3302     
- Misses      31711    32543     +832     
+ Partials      116        0     -116

see 1065 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efced74...c3ebead. Read the comment docs.

nicolecomputer · 2024-08-29T17:07:55Z

The parent pull-request (#1567) has been merged into main, but this branch (new/remove-points-keyboard) now has conflicts with the new base branch. These conflicts must be resolved before checks can complete on this pull-request.

github-actions · 2024-08-29T18:30:22Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (c3ebead) and published it to npm. You
can install it using the tag PR1570.

Example:

yarn add @khanacademy/perseus@PR1570

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1570

benchristel

The code looks good, and I love the way it works. But I'm wondering if there's a simpler way to accomplish this (with no added reducer state). I left comments inline.

benchristel · 2024-08-30T00:27:35Z

packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

                    }}
+                    ref={graphRef}
+                    tabIndex={0}


Should the graph be focusable with the tab key? If we want it to be programmatically focusable but not tabbable, we could set tabIndex={-1}. (MDN docs)

@mark-fitzgerald and @nishasy thought it should be 0 because we expect it to be tab-able. Nisha has future work that uses that!

benchristel · 2024-08-30T00:52:54Z

packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

+                        if (event.key === "Backspace") {
+                            dispatch(actions.global.deleteIntent());
+                            graphRef.current?.focus();
+                        }


I'm still a little confused about why we have to handle this event on the graph component rather than on the point itself. I tried adding an onKeyDown handler to the g[data-testid='movable-point'] element and it works in both Edge and Safari. The focused point's event handler is fired when you press a key.

The advantage of putting the event handler on the point would be that we wouldn't have to keep track of which point is focused in the useReducer state.

Am I missing something?

I'm definitely open to changing it but there are a couple of things I like:

This lets us add a delete intent to other kinds of graphs with no fuss. I think we'll use that for unlimited polygon (but that's a gut feeling).

This makes it natural to add other keyboard shortcuts and gives a natural place for them to live

I think I need to know the selected point regardless of this change. After a point is added we want to focus on the newly added point. So as a learner I tab to the "Add point" button, hit space, the point is selected and I can manipulate it with my keyboard. I think that means we'll need to keep track of the selected point so that after the add button is activated we select the right thing.

benchristel

LGTM!

@handeyeco

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/[email protected] ### Major Changes - [#1577](#1577) [`c875acd01`](c875acd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove example widgets and their editors ### Minor Changes - [#1570](#1570) [`c4432ffad`](c4432ff) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Remove unlimited points via keyboard - [#1582](#1582) [`377b7ce68`](377b7ce) Thanks [@aemandine](https://github.com/aemandine)! - Add save warnings to PhET widget editor and un-hide widget from content editor widget dropdown ### Patch Changes - [#1578](#1578) [`78bb8573e`](78bb857) Thanks [@aemandine](https://github.com/aemandine)! - Remove simpleValidate from PhET widget - [#1585](#1585) [`a6ec402c0`](a6ec402) Thanks [@handeyeco](https://github.com/handeyeco)! - Reorganize files in the widgets folder - [#1589](#1589) [`d56952564`](d569525) Thanks [@aemandine](https://github.com/aemandine)! - Make PhET widget smaller - [#1587](#1587) [`8015cdefb`](8015cde) Thanks [@aemandine](https://github.com/aemandine)! - Tidying up PhET widget - [#1583](#1583) [`615567bd2`](615567b) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove sort-comp exceptions and reorder components ## @khanacademy/[email protected] ### Major Changes - [#1577](#1577) [`c875acd01`](c875acd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove example widgets and their editors ### Minor Changes - [#1582](#1582) [`377b7ce68`](377b7ce) Thanks [@aemandine](https://github.com/aemandine)! - Add save warnings to PhET widget editor and un-hide widget from content editor widget dropdown ### Patch Changes - [#1585](#1585) [`a6ec402c0`](a6ec402) Thanks [@handeyeco](https://github.com/handeyeco)! - Reorganize files in the widgets folder - [#1587](#1587) [`8015cdefb`](8015cde) Thanks [@aemandine](https://github.com/aemandine)! - Tidying up PhET widget - [#1583](#1583) [`615567bd2`](615567b) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove sort-comp exceptions and reorder components - Updated dependencies \[[`78bb8573e`](78bb857), [`a6ec402c0`](a6ec402), [`d56952564`](d569525), [`c875acd01`](c875acd), [`8015cdefb`](8015cde), [`c4432ffad`](c4432ff), [`615567bd2`](615567b), [`377b7ce68`](377b7ce)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1585](#1585) [`a6ec402c0`](a6ec402) Thanks [@handeyeco](https://github.com/handeyeco)! - Reorganize files in the widgets folder

Nicole Watts added 6 commits August 22, 2024 15:11

wip: SUPER SUPER rough

1a4b68a

wip: reducer tests

7793c9b

revert

547e475

wip: reducer action

da674c0

Merge branch 'main' into new/remove-points-reducer

1b28d3c

wip: remove points

9b4efea

nicolecomputer self-assigned this Aug 28, 2024

khan-actions-bot requested a review from a team August 28, 2024 17:39

wip: changeset

2f5244f

wip: add tests

bfcdc91

nicolecomputer changed the base branch from new/remove-points-reducer to main August 29, 2024 17:07

Nicole Watts added 3 commits August 29, 2024 10:41

Merge branch 'main' into new/remove-points-keyboard

45f0eec

Merge branch 'main' into new/remove-points-keyboard

762040d

wip: cleanup

e484b2a

focus the graph after the point is removed

2526d20

nicolecomputer changed the title ~~WIP: Remove unlimited points via keyboard~~ Remove unlimited points via keyboard Aug 29, 2024

remove unused token

cfabf21

nicolecomputer requested review from benchristel and mark-fitzgerald August 29, 2024 22:00

wip: fixup tests

c3ebead

benchristel reviewed Aug 30, 2024

View reviewed changes

nicolecomputer requested a review from benchristel September 3, 2024 17:41

benchristel approved these changes Sep 3, 2024

View reviewed changes

nicolecomputer merged commit c4432ff into main Sep 3, 2024
12 of 13 checks passed

nicolecomputer deleted the new/remove-points-keyboard branch September 3, 2024 19:46

khan-actions-bot mentioned this pull request Sep 3, 2024

Version Packages #1581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unlimited points via keyboard #1570

Remove unlimited points via keyboard #1570

nicolecomputer commented Aug 28, 2024 •

edited

Loading

khan-actions-bot commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading

codecov bot commented Aug 28, 2024 •

edited

Loading

nicolecomputer commented Aug 29, 2024

github-actions bot commented Aug 29, 2024 •

edited

Loading

benchristel left a comment

benchristel Aug 30, 2024

nicolecomputer Sep 3, 2024

benchristel Aug 30, 2024

nicolecomputer Sep 3, 2024

benchristel left a comment

Remove unlimited points via keyboard #1570

Remove unlimited points via keyboard #1570

Conversation

nicolecomputer commented Aug 28, 2024 • edited Loading

Summary:

Test plan:

khan-actions-bot commented Aug 28, 2024 • edited Loading

Gerald

github-actions bot commented Aug 28, 2024 • edited Loading

codecov bot commented Aug 28, 2024 • edited Loading

Codecov Report

nicolecomputer commented Aug 29, 2024

github-actions bot commented Aug 29, 2024 • edited Loading

npm Snapshot: Published

benchristel left a comment

Choose a reason for hiding this comment

benchristel Aug 30, 2024

Choose a reason for hiding this comment

nicolecomputer Sep 3, 2024

Choose a reason for hiding this comment

benchristel Aug 30, 2024

Choose a reason for hiding this comment

nicolecomputer Sep 3, 2024

Choose a reason for hiding this comment

benchristel left a comment

Choose a reason for hiding this comment

nicolecomputer commented Aug 28, 2024 •

edited

Loading

khan-actions-bot commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading

codecov bot commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 29, 2024 •

edited

Loading