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

186143109 drawing toolbar in new toolbar system #2309

Merged
merged 42 commits into from
May 29, 2024

Conversation

bacalj
Copy link
Contributor

@bacalj bacalj commented May 20, 2024

PT-186143109

This reimplements that drawing tile toolbar using the general toolbar system

  • migrates existing buttons to new system
  • creates ToolbarButtonSvg - a component that is just the svg portion of a button that will accept svg settings and can be placed within the general TileToolbarButton
  • moves (and simplifies) palette state to volatile
  • moves and consolidates toolbar styles
  • creates a new context for the calculations necessary for adding images, previously passed through props
  • implements and supports a version of the new general upload button from at-this-time unmerged Geometry toolbar #2296
  • app-config updated to include default buttons
  • unit level buttons will still apply
  • cypress selectors changed

bacalj and others added 2 commits May 20, 2024 16:25
- update app config
- add unused onTouchHold to TileToolbarButtonProps
- registration
- create modal buttons
- stub out action-on-drawing buttons
- comment out stayles that will need to come back in, stub new styles file
- remove some functionality from drawing-tile, to be added back
- set up new drawing-toolbar-buttons file, leave old one in for reference
Copy link

cypress bot commented May 20, 2024

Passing run #12685 ↗︎

0 105 4 0 Flakiness 0

Details:

null
Project: collaborative-learning Commit: ad45db2f1d
Status: Passed Duration: 12:39 💡
Started: May 28, 2024 9:05 PM Ended: May 28, 2024 9:17 PM

Review all test suite changes for PR #2309 ↗︎

Copy link

codecov bot commented May 20, 2024

Codecov Report

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

Project coverage is 84.43%. Comparing base (a02e958) to head (ad45db2).

Files Patch % Lines
src/components/toolbar/upload-button.tsx 73.33% 4 Missing ⚠️
src/plugins/drawing/action-buttons.tsx 93.02% 3 Missing ⚠️
...ns/drawing/toolbar-buttons/image-upload-button.tsx 78.57% 3 Missing ⚠️
...plugins/drawing/toolbar-buttons/select-buttons.tsx 92.85% 3 Missing ⚠️
.../plugins/drawing/toolbar-buttons/vector-button.tsx 91.42% 3 Missing ⚠️
...c/plugins/drawing/toolbar-buttons/stamp-button.tsx 94.44% 2 Missing ⚠️
src/components/toolbar/tile-toolbar-button.tsx 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2309      +/-   ##
==========================================
- Coverage   84.51%   84.43%   -0.09%     
==========================================
  Files         705      711       +6     
  Lines       36007    36033      +26     
  Branches     9190     9195       +5     
==========================================
- Hits        30432    30423       -9     
- Misses       5256     5290      +34     
- Partials      319      320       +1     
Flag Coverage Δ
cypress-regression 74.87% <93.33%> (-0.12%) ⬇️
cypress-smoke 29.36% <51.55%> (-0.12%) ⬇️
jest 48.35% <42.37%> (-0.29%) ⬇️

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.

- uses Boris' general button from PR #2296
- updates general toolbar to support above
- keeps current image upload mechanism by adding state var to context
- vectorType needs to be passed to svg as vectortype
- strokeDashArray needs to be strokeDasharray
@bacalj bacalj marked this pull request as ready for review May 28, 2024 11:22
Copy link
Contributor

@bgoldowsky bgoldowsky left a comment

Choose a reason for hiding this comment

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

As noted in story hour, touch-hold should just be equivalent to click on buttons that don't have a separate action (eg, fill color).


const tipOptions = useTooltipOptions();
const { onTouchStart, onTouchEnd, onMouseDown, onMouseUp, onClick: handleOnClick } = useTouchHold(
() => onTouchHold?.({} as React.MouseEvent),
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like either onTouchHold should be changed to not expect an argument, or an actual event argument should be passed.

src/plugins/drawing/action-buttons.tsx Show resolved Hide resolved
src/plugins/drawing/action-buttons.tsx Outdated Show resolved Hide resolved
src/plugins/drawing/components/drawing-area-context.tsx Outdated Show resolved Hide resolved
src/plugins/drawing/toolbar-buttons/toolbar-button-svg.tsx Outdated Show resolved Hide resolved
src/plugins/drawing/drawing-registration.ts Outdated Show resolved Hide resolved
- if multiple objects are selected and any are a group, enable ungroup button
- shared data buttons should be registered from shared data
- fix imports
- cleanup
bacalj and others added 3 commits May 28, 2024 16:39
- in cases where user might expect long click to work because there is a dropdown
Copy link
Contributor

@bgoldowsky bgoldowsky 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!

@bacalj bacalj requested a review from johnTcrawford May 29, 2024 14:35
@scytacki scytacki removed the request for review from johnTcrawford May 29, 2024 16:06
@scytacki scytacki merged commit a90ca91 into master May 29, 2024
15 of 17 checks passed
@scytacki scytacki deleted the 186143109-drawing-toolbar-update branch May 29, 2024 16:07
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.

4 participants