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 toolbar #2296

Closed
wants to merge 12 commits into from
Closed

Geometry toolbar #2296

wants to merge 12 commits into from

Conversation

bgoldowsky
Copy link
Contributor

@bgoldowsky bgoldowsky commented May 8, 2024

This moves the geometry toolbar to the common toolbar framework, and moves it part of the way towards the new spec for now it should work. See notes in PT-186143166.

  • Point button is added. For the moment point mode just removes the double-click-to-create-a-polygon behavior.
  • Icons and order of buttons updated.
  • Add data is restyled and moved from the title area to the toolbar.
  • Movable line is removed.

Cypress test note -- existing Cypress tests, somehow, passed with these changes. I think that means they are not very comprehensive. I deleted some unit tests for toolbar buttons since they were no longer defined in the same way. Geometry Cypress tests are improved in the next PR (#2303 ).

This has been merged to geometry-update branch

Copy link

cypress bot commented May 8, 2024

1 failed test on run #12580 ↗︎

1 104 4 0 Flakiness 0

Details:

null
Project: collaborative-learning Commit: 73b4fb454d
Status: Failed Duration: 13:11 💡
Started: May 23, 2024 11:21 AM Ended: May 23, 2024 11:34 AM
Failed  cypress/e2e/functional/tile_tests/arrow_annotation_spec.js • 1 failed test • Regression tests

View Output

Test Artifacts
Arrow Annotations (Sparrows) > can add arrows to geometry tiles Test Replay Screenshots

Review all test suite changes for PR #2296 ↗︎

Copy link

codecov bot commented May 8, 2024

Codecov Report

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

Project coverage is 79.15%. Comparing base (243c405) to head (73b4fb4).

Files Patch % Lines
...s/tiles/geometry/geometry-toolbar-registration.tsx 82.19% 13 Missing ⚠️
src/components/toolbar/upload-button.tsx 73.33% 4 Missing ⚠️
...components/tiles/geometry/geometry-tile-context.ts 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2296      +/-   ##
==========================================
- Coverage   84.51%   79.15%   -5.37%     
==========================================
  Files         705      706       +1     
  Lines       36004    36021      +17     
  Branches     9189     9190       +1     
==========================================
- Hits        30429    28511    -1918     
- Misses       5256     7063    +1807     
- Partials      319      447     +128     
Flag Coverage Δ
cypress ?
cypress-regression 68.55% <77.21%> (-6.43%) ⬇️
cypress-smoke 29.47% <73.41%> (-0.01%) ⬇️
jest 48.52% <44.03%> (-0.12%) ⬇️

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 marked this pull request as ready for review May 10, 2024 14:05
@bgoldowsky bgoldowsky requested a review from scytacki May 10, 2024 14:26
Copy link
Member

@scytacki scytacki 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 to me. I just left one comment about a FIXME. If it is still unresolved it is fine to leave it, I just wasn't sure.

const selectedPoints = selectedObjects?.filter(isPoint);
const selectedPoint = selectedPoints?.length === 1 ? selectedPoints[0] : undefined;
const disableVertexAngle = !(selectedPoint && canSupportVertexAngle(selectedPoint));
// FIXME toggling this doesn't trigger the "observer" and thus doesn't change the selected state
Copy link
Member

Choose a reason for hiding this comment

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

Is this still a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reminding me of that. I put in a fix but it seems a bit hacky - can you review and LMK if you see a cleaner way to do it?

bacalj added a commit that referenced this pull request May 26, 2024
- uses Boris' general button from PR #2296
- updates general toolbar to support above
- keeps current image upload mechanism by adding state var to context
@bgoldowsky bgoldowsky marked this pull request as draft May 29, 2024 19:46
@bgoldowsky bgoldowsky closed this Jul 10, 2024
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.

2 participants