Skip to content
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

Update Polygon graphs to have weighted lines only after focus from keyboard #1455

Merged
merged 19 commits into from
Aug 1, 2024

Conversation

Myranae
Copy link
Contributor

@Myranae Myranae commented Jul 26, 2024

Summary:

Previously, Polygon had its shape weighted upon hover and after clicking in the shape. This is not how circle is currently working. To bring them into alignment, this changes Polygon to become weighted only when the shape is tabbed to by the keyboard or if the shape is moved via keyboard after the user clicks on it.

Before

Screen.Recording.2024-07-26.at.2.20.56.PM.mov

After

Screen.Recording.2024-08-01.at.1.36.39.PM.mov

Issue: LEMS-2200

Test plan:

  • Checkout the associated branch
  • Run yarn dev
  • On the gallery page, turn on the flag for polygon and circle
  • Confirm Polygon does not have weighted lines on hover or when dragging. After clicking the shape, confirm the shape becomes weighted when you move it with the arrow keys. Confirm the lines return to normal after clicking outside of the shape.
  • Confirm this functionality matches Circle graphs

@Myranae Myranae self-assigned this Jul 26, 2024
Copy link
Contributor

github-actions bot commented Jul 26, 2024

Size Change: +327 B (+0.04%)

Total Size: 852 kB

Filename Size Change
packages/perseus/dist/es/index.js 413 kB +327 B (+0.08%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.3 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.8 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-editor/dist/es/index.js 272 kB
packages/perseus-linter/dist/es/index.js 21.6 kB
packages/perseus/dist/es/strings.js 3.23 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.46%. Comparing base (ae8f208) to head (603fc30).

Files Patch % Lines
.../src/widgets/interactive-graphs/graphs/polygon.tsx 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1455      +/-   ##
==========================================
+ Coverage   69.94%   70.46%   +0.51%     
==========================================
  Files         509      512       +3     
  Lines      105417   105477      +60     
  Branches     7579    10745    +3166     
==========================================
+ Hits        73736    74326     +590     
+ Misses      31495    31151     -344     
+ Partials      186        0     -186     

Impacted file tree graph

Files Coverage Δ
...tive-graphs/graphs/components/angle-indicators.tsx 89.15% <ø> (-0.27%) ⬇️
.../src/widgets/interactive-graphs/graphs/polygon.tsx 90.47% <96.55%> (+0.82%) ⬆️

... and 144 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 ae8f208...603fc30. Read the comment docs.

@Myranae Myranae marked this pull request as ready for review July 26, 2024 21:04
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Jul 26, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/cyan-seals-yawn.md, packages/perseus/src/widgets/interactive-graphs/graphs/polygon.test.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.tsx

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

@khan-actions-bot khan-actions-bot requested a review from a team July 26, 2024 21:04
Copy link
Contributor

github-actions bot commented Jul 29, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1455

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

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

Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that it works, but I don't fully understand how it works. It seems like this relies on the order in which events fire? (onMouseDown, onFocus, onMouseUp)? The focused line weight does not appear if you hold down the mouse button while tabbing through the elements. I don't know if that's a real problem, though.

I prototyped an alternative implementation that I think covers all the same use cases: #1458. It uses :focus-visible, so it will use the the browser's logic for deciding when focus should be shown. What do you think about this approach?

Myranae added 2 commits July 30, 2024 10:53
No longer use order of events to control styling. Switch to referencing when the element has :focus-visible.
@khan-actions-bot khan-actions-bot requested a review from a team July 30, 2024 15:59
@Myranae Myranae requested a review from SonicScrewdriver July 30, 2024 15:59
@Myranae
Copy link
Contributor Author

Myranae commented Jul 30, 2024

Thanks, @benchristel ! I'm not so comfortable with refs here, but that is exactly what I needed! I don't think I would have known about jsdom not supporting :focus-visible even if I had used a ref, so thanks for sharing that try/catch block in the function you shared. Going to try to add in a test before finalizing this, unless you don't think that's necessary. Updated this PR with your suggestion ❤️

Myranae added 2 commits August 1, 2024 10:36
Fixes it so that tabbing to the element darkens the lines and then clicking outside of the element returns them to normal
@Myranae
Copy link
Contributor Author

Myranae commented Aug 1, 2024

@benchristel I was able to use your idea, but I had to add some more logic around user actions to get it to do everything we need it to do. Would you mind taking another look and letting me know what you think? It seems like the main issue might be getting the line weighting / shape to re-render when certain changes occur (like blur or tabbing). I used state to do that, but would love another suggestion more in-line with your alternative approach.

@Myranae Myranae requested a review from benchristel August 1, 2024 19:01
Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I think I like this better than my original approach.

@Myranae Myranae merged commit 8b02682 into main Aug 1, 2024
11 checks passed
@Myranae Myranae deleted the tb/LEMS-2200/remove-line-weight-after-moving-polygon branch August 1, 2024 20:24
Myranae pushed a commit that referenced this pull request Aug 9, 2024
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

-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead

### Minor Changes

-   [#1455](#1455) [`8b0268215`](8b02682) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon graphs to have weighted lines only on keyboard focus


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1428](#1428) [`eb9f3f9c0`](eb9f3f9) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Drop katex dependency - no longer used


-   [#1472](#1472) [`4c2ace57a`](4c2ace5) Thanks [@benchristel](https://github.com/benchristel)! - Add keyboard controls to Mafs angle graphs


-   [#1487](#1487) [`53031f8df`](53031f8) Thanks [@Myranae](https://github.com/Myranae)! - Make graphs non-interactive via keyboard when their question has been answered correctly


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1474](#1474) [`a8aaac339`](a8aaac3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor type fixes in the `ImageLoader` and `SvgImage` components


-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1475](#1475) [`6a218bcc1`](6a218bc) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - add non-visual text as a description for expression widget

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Minor Changes

-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1491](#1491) [`395b44f29`](395b44f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Correct flag logic for polygon graph start coords UI


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1471](#1471) [`b4e0b60dc`](b4e0b60) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Fix the dropdowns overlapping labels in locked figures settings


-   [#1481](#1481) [`a35be719f`](a35be71) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Stop page scrolling on number text field focused scroll


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1477](#1477) [`653b520c6`](653b520) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Migrate ItemExtrasEditor to WB checkboxes

-   Updated dependencies \[[`a8aaac339`](a8aaac3), [`de13fd337`](de13fd3), [`8b0268215`](8b02682), [`2ebbc1978`](2ebbc19), [`6c6ff52f4`](6c6ff52), [`18f38fca4`](18f38fc), [`ef96cdd54`](ef96cdd), [`342a72211`](342a722), [`de2883b3f`](de2883b), [`7fd586ef4`](7fd586e), [`f920a4cc7`](f920a4c), [`eb9f3f9c0`](eb9f3f9), [`4c2ace57a`](4c2ace5), [`53031f8df`](53031f8), [`808956098`](8089560), [`5e66539e6`](5e66539), [`0b625f560`](0b625f5), [`6a218bcc1`](6a218bc), [`0bec013e8`](0bec013)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

-   [#1495](#1495) [`6c6ff52f4`](6c6ff52) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove old buttons that we weren't using anymore


-   [#1475](#1475) [`342a72211`](342a722) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update wonder blocks popover versions


-   [#1496](#1496) [`5e66539e6`](5e66539) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove unused buttons from MathInput; add Lato

## @khanacademy/[email protected]

### Patch Changes

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

Author: khan-actions-bot

Reviewers: Myranae

Required Reviewers:

Approved By: Myranae

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️  Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants