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

Add points by hand toolbar button #2213

Merged
merged 12 commits into from
Mar 15, 2024
Merged

Add points by hand toolbar button #2213

merged 12 commits into from
Mar 15, 2024

Conversation

bgoldowsky
Copy link
Contributor

@bgoldowsky bgoldowsky commented Feb 29, 2024

Adds the ability for the Graph tile to have its own dataset.

As specified in PT-186610168, adds a toolbar button that creates a dataset attached to the graph, and a layer for it. Two attributes are created and set up to be plotted. Subsequent work will allow points to be added. The name of the new dataset can be edited.

Copy link

cypress bot commented Feb 29, 2024

1 flaky test on run #11659 ↗︎

0 95 5 0 Flakiness 1

Details:

null
Project: collaborative-learning Commit: 5b0b81c7e4
Status: Passed Duration: 08:37 💡
Started: Mar 14, 2024 4:45 PM Ended: Mar 14, 2024 4:54 PM
Flakiness  cypress/e2e/functional/tile_tests/drawing_tool_spec.js • 1 flaky test

View Output

Test Artifacts
Draw Tool Tile > Text Test Replay Screenshots

Review all test suite changes for PR #2213 ↗︎

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

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

Project coverage is 83.45%. Comparing base (41128d7) to head (5b0b81c).
Report is 7 commits behind head on master.

Files Patch % Lines
...omponents/utilities/editable-label-with-button.tsx 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2213      +/-   ##
==========================================
+ Coverage   83.42%   83.45%   +0.02%     
==========================================
  Files         688      689       +1     
  Lines       34075    34138      +63     
  Branches     8836     8848      +12     
==========================================
+ Hits        28428    28490      +62     
- Misses       5352     5353       +1     
  Partials      295      295              
Flag Coverage Δ
cypress 73.16% <100.00%> (+0.04%) ⬆️
cypress-regression 73.16% <100.00%> (+0.03%) ⬆️
cypress-smoke 30.07% <7.14%> (-0.05%) ⬇️
jest 51.99% <60.00%> (+0.05%) ⬆️

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 March 1, 2024 20:18
@@ -598,7 +642,7 @@ export const GraphModel = TileContentModel
(ids) => {
ids.forEach(id => {
if (!self._idColors.has(id)) {
self.setColorForIdWithoutUndo(id, self.nextColor);
self.setColorForId(id, self.nextColor);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may require a more subtle fix, but I was getting warnings about withoutUndo being called as a child action.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah there is a option you pass to withoutUndo so it basically doesn't print this warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I changed it back to setColorForIdWithoutUndo and added the unlessChildAction option there.

@bgoldowsky bgoldowsky requested a review from scytacki March 1, 2024 21:32
Base automatically changed from 186549943-dataset-naming to master March 1, 2024 21:54
@scytacki
Copy link
Member

scytacki commented Mar 5, 2024

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.

@bgoldowsky bgoldowsky added the pending QA review A PR that is waiting for QA to review it before merging. label Mar 5, 2024
Copy link
Contributor

@johnTcrawford johnTcrawford left a comment

Choose a reason for hiding this comment

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

Cypress code looks good. (I'll test the new functionality shortly.)

@scytacki scytacki added approved QA review and removed pending QA review A PR that is waiting for QA to review it before merging. labels Mar 15, 2024
@scytacki
Copy link
Member

I see based on the PT story that the functionality was approved. I'm going to merge this.

@scytacki scytacki merged commit 765fcae into master Mar 15, 2024
24 of 25 checks passed
@scytacki scytacki deleted the 186610168-manual-points branch March 15, 2024 13:28
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