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

Geometry: Select, Points, and Polygon modes #2303

Merged
merged 25 commits into from
May 29, 2024

Conversation

bgoldowsky
Copy link
Contributor

@bgoldowsky bgoldowsky commented May 10, 2024

This is the combined changes from PT-187496223, PT-187496261, and some straggling changes related to PT-186143166 (which was mostly complete in PR #2296).

In points and polygon modes, a "phantom point" is introduced that tracks the mouse pointer. On a click, this becomes a real point.

Logging for all Geometry tile actions was updated to be consistent and account for the new UI.

A minimal Cypress tests of actions in each mode is included; but testing for the Geometry tile could still be improved.

@bgoldowsky bgoldowsky changed the base branch from master to 186143166-geometry-toolbar May 10, 2024 21:27
Copy link

cypress bot commented May 10, 2024

Passing run #12706 ↗︎

0 3 0 0 Flakiness 0

Details:

null
Project: collaborative-learning Commit: 9198b7dbf1
Status: Passed Duration: 02:12 💡
Started: May 29, 2024 7:20 PM Ended: May 29, 2024 7:23 PM

Review all test suite changes for PR #2303 ↗︎

Copy link

codecov bot commented May 10, 2024

Codecov Report

Attention: Patch coverage is 85.05747% with 52 lines in your changes are missing coverage. Please review.

Project coverage is 84.41%. Comparing base (73b4fb4) to head (9198b7d).

Files Patch % Lines
src/models/tiles/geometry/geometry-content.ts 80.48% 31 Missing and 1 partial ⚠️
src/components/tiles/geometry/geometry-content.tsx 78.49% 17 Missing and 3 partials ⚠️
Additional details and impacted files
@@                      Coverage Diff                       @@
##           186143166-geometry-toolbar    #2303      +/-   ##
==============================================================
+ Coverage                       79.15%   84.41%   +5.26%     
==============================================================
  Files                             706      706              
  Lines                           36021    36994     +973     
  Branches                         9190     9604     +414     
==============================================================
+ Hits                            28511    31229    +2718     
+ Misses                           7063     5467    -1596     
+ Partials                          447      298     -149     
Flag Coverage Δ
cypress 27.64% <77.54%> (?)
cypress-regression 75.11% <85.18%> (+6.55%) ⬆️
cypress-smoke 29.01% <10.69%> (-0.47%) ⬇️
jest 48.55% <55.80%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bgoldowsky bgoldowsky requested a deployment to branches May 13, 2024 19:23 — with GitHub Actions Abandoned
@bgoldowsky bgoldowsky changed the base branch from 186143166-geometry-toolbar to master May 20, 2024 21:51
@bgoldowsky bgoldowsky changed the base branch from master to 186143166-geometry-toolbar May 22, 2024 13:26
@bgoldowsky bgoldowsky mentioned this pull request May 22, 2024
@bgoldowsky bgoldowsky changed the title Geometry: Select mode and Points mode Geometry: Select, Points, and Polygon modes May 22, 2024
@bgoldowsky bgoldowsky marked this pull request as ready for review May 23, 2024 13:39
@bgoldowsky bgoldowsky requested a review from kswenson May 23, 2024 13:39
Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Looks good -- just a handful of minor suggestions

cypress/e2e/functional/tile_tests/arrow_annotation_spec.js Outdated Show resolved Hide resolved
cypress/e2e/functional/tile_tests/geometry_tool_spec.js Outdated Show resolved Hide resolved
src/utilities/js-utils.ts Outdated Show resolved Hide resolved
src/models/tiles/geometry/jsxgraph.d.ts Outdated Show resolved Hide resolved
src/models/tiles/geometry/geometry-utils.ts Outdated Show resolved Hide resolved
src/models/tiles/geometry/geometry-content.ts Outdated Show resolved Hide resolved
src/components/tiles/geometry/geometry-content.tsx Outdated Show resolved Hide resolved
Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Changes look good

expect(content.getDependents(["p1"], { required: true })).toEqual(["p1"]);
expect(content.getDependents(["p3"])).toEqual(["p3", polygonId]);
expect(content.getDependents(["p3"], { required: true })).toEqual(["p3"]);
const polygonId = polygon?.id;
Copy link
Member

Choose a reason for hiding this comment

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

FYI, another approach here would be to assert that it's defined:

Suggested change
const polygonId = polygon?.id;
const polygonId = polygon!.id;

And then you wouldn't have to test for it below. Not something I'd recommend in production code but I think it's fine in tests.

@bgoldowsky bgoldowsky changed the base branch from 186143166-geometry-toolbar to geometry-update May 29, 2024 19:40
@bgoldowsky bgoldowsky removed the request for review from johnTcrawford May 29, 2024 19:41
@bgoldowsky bgoldowsky merged commit fc294ea into geometry-update May 29, 2024
20 checks passed
@bgoldowsky bgoldowsky deleted the 187496223-geometry-select branch May 29, 2024 19:44
@scytacki
Copy link
Member

This PR is part of the 5.12.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants