feat: drawing tile navigator panel (PT-185987214) #2416

emcelroy · 2024-09-30T14:52:53Z

The tile navigator panel is a scaled-down clone of the tile content area. The panel can be added or removed using a new toolbar button.

For now the panel is only an option for the drawing tile, and it only shows the content of the tile. Panning buttons will be added to the navigator in a separate piece of work.

codecov · 2024-09-30T14:57:04Z

Codecov Report

Attention: Patch coverage is 95.57522% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.29%. Comparing base (8bef86c) to head (e50c2a4).
Report is 27 commits behind head on master.

Files with missing lines	Patch %	Lines
src/models/tiles/navigatable-tile-model.ts	84.21%	3 Missing ⚠️
src/components/tiles/tile-navigator.tsx	98.36%	1 Missing ⚠️
src/components/toolbar/navigator-button.tsx	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2416      +/-   ##
==========================================
+ Coverage   85.38%   86.29%   +0.91%     
==========================================
  Files         742      745       +3     
  Lines       38379    38485     +106     
  Branches     9801     9823      +22     
==========================================
+ Hits        32769    33212     +443     
+ Misses       5304     4972     -332     
+ Partials      306      301       -5

Flag	Coverage Δ
cypress-regression	`78.14% <95.65%> (+1.48%)`	⬆️
cypress-smoke	`28.08% <73.91%> (+0.12%)`	⬆️
jest	`48.81% <35.39%> (-0.06%)`	⬇️

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.

cypress · 2024-09-30T14:57:37Z

collaborative-learning Run #13981

Run Properties: Passed #13981 • e50c2a4810: chore: remove IDrawingTileProps import

Project	`collaborative-learning`
Run status	`Passed #13981`
Run duration	`14m 47s`
Commit	`e50c2a4810: chore: remove IDrawingTileProps import`
Committer	`Ethan McElroy`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`3`
Skipped	`0`
Passing	`115`

[#185987214](https://www.pivotaltracker.com/story/show/185987214)

src/plugins/drawing/model/drawing-content.ts

bgoldowsky

The current implementation shows a rendundant duplicate of whatever portion of the drawing canvas is currently visible in the main tile. The spec is rather unclear, but I do not think that is what we want.

I think we want to make this work like the Diagram tile, where the navigator panel always shows all the content, even if the main tile is zoomed in so that some content is invisible. In that case, a box is drawn within the navigator to show what portion of the content is visible. Here is a screenshot of the diagram tile showing what I mean. It's not super obvious because of the low contrast grey-on-grey, but you can see the two full "variable" boxes in the navigator panel, even though they are not fully visible in the main tile.

cypress/e2e/functional/tile_tests/tile_navigator_spec.js

cypress/e2e/functional/document_tests/copy_doc_test_spec.js

cypress/support/elements/tile/TileNavigator.js

src/components/tiles/tile-navigator.tsx

src/components/toolbar/navigator-button.tsx

src/plugins/drawing/model/drawing-content.ts

src/components/tiles/tile-navigator.tsx

emcelroy · 2024-10-01T20:11:00Z

The current implementation shows a rendundant duplicate of whatever portion of the drawing canvas is currently visible in the main tile. The spec is rather unclear, but I do not think that is what we want.

I think we want to make this work like the Diagram tile, where the navigator panel always shows all the content, even if the main tile is zoomed in so that some content is invisible. In that case, a box is drawn within the navigator to show what portion of the content is visible. Here is a screenshot of the diagram tile showing what I mean. It's not super obvious because of the low contrast grey-on-grey, but you can see the two full "variable" boxes in the navigator panel, even though they are not fully visible in the main tile.

Thanks. I thought I got approval on the current arrangement, but now I'm not so sure. Let me double check.

…nder prop instead of cloneElement, comments

This uses applySnapshot to update the class users in place. This will make code using these users more efficient since they can just observer the user properties for changes.

Original the MST environment was avoided because I was planning to switch this store to just be MobX. However the group store needs to be synced with the data from the database, and it is easier to do that using MST than MobX.

This uses applySnapshot to sync the changes from the DB. This way the existing objects are updated instead of creating new ones. I also missed a change in stores when committing the last set of changes.

This will allow us to update the users later and have the group views automatically update without having to also update the group user objects. This also removes the option to skip unknown students. They are part of the group so we should show them even if we don't know who they are yet.

We use a timestamp for when the class information is retrieved from the portal. And compare this to the timestamp in the groupUser to figure out if this groupUser is older than the class info or newer than the class info.

The portal service object provides a place to store portal info like JWTs, and methods that use and update this info. This new object is used to refresh the class info if necessary after updating the group info from Firebase.

bgoldowsky

This looks good!
The only thing that's important to fix is that there shouldn't be any imports that cause plugin code to be loaded from non-plugin code, see specific comments below.

bgoldowsky · 2024-10-04T20:47:34Z

cypress/e2e/functional/tile_tests/tile_navigator_spec.js

+
+    cy.log("Move tile navigator to the top of the drawing tile in a quick animation");
+    tileNavigator.getTileNavigatorPlacementButton().click();
+    cy.wait(300);


You shouldn't need the explicit wait here (or in line 70), since Cypress will automatically wait up to its timeout for a condition to become true.

I know that's true generally, but found these were required to make the test pass consistently. I believe it has something to do with how the component waits for the animation to complete before updating the model which is what causes the class name to change. I'm not sure why explicitly setting a wait makes it always work. I did spend some time looking into it but wasn't able to figure out a way to make them unnecessary. For now at least I'd like to leave them so I can complete the main task.

bgoldowsky · 2024-10-04T20:51:57Z

cypress/support/elements/tile/DrawToolTile.js

@@ -6,10 +6,11 @@ class DrawToolTile{
      return cy.get(`${workspaceClass || ".primary-workspace"} .editable-tile-title-text`);
    }
    getDrawTileComponent(){
-      return cy.get('.primary-workspace [data-testid=drawing-tool]');
+      return cy.get('.primary-workspace [data-testid=drawing-tool]')
+               .not('[data-testid="tile-navigator"] [data-testid=drawing-tool]');


Any reason why some use quotes inside the brackets and some don't?

No, that was just me being a bit inconsistent. Since they're not needed, I've removed them. Thanks for catching that.

bgoldowsky · 2024-10-08T12:57:07Z

src/components/tiles/tile-navigator.tsx

+import { BoundingBox } from "../../plugins/drawing/model/drawing-basic-types";
+import { IDrawingTileProps } from "../../plugins/drawing/components/drawing-tile";


There should be no dependencies from core (non-plugin) code into plugins, this will cause plugins to be loaded when they are not needed.

bgoldowsky · 2024-10-08T13:02:21Z

src/models/tiles/navigatable-tile-model.ts

@@ -0,0 +1,45 @@
+import { types, Instance, SnapshotIn } from "mobx-state-tree";
+
+import { BoundingBox } from "../../plugins/drawing/model/drawing-basic-types";


Remove this dependency on a plugin.

bgoldowsky · 2024-10-08T15:27:43Z

src/components/tiles/tile-navigator.tsx

+
+import { ITileProps } from "./tile-component";
+import { BoundingBox, NavigatableTileModelType } from "../../models/tiles/navigatable-tile-model";
+import { IDrawingTileProps } from "../../plugins/drawing/components/drawing-tile";


One plugin dependency still to remove.

bgoldowsky

Changes look good 👍

emcelroy temporarily deployed to branches September 30, 2024 14:57 — with GitHub Actions Inactive

emcelroy added the run regression label Sep 30, 2024

feat: drawing tile navigator panel (PT-185987214)

e134304

[#185987214](https://www.pivotaltracker.com/story/show/185987214)

emcelroy force-pushed the 185987214-drawing-tile-navigator-panel branch from 78b3d4a to e134304 Compare September 30, 2024 15:24

emcelroy temporarily deployed to branches September 30, 2024 15:28 — with GitHub Actions Inactive

emcelroy commented Sep 30, 2024

View reviewed changes

src/plugins/drawing/model/drawing-content.ts Outdated Show resolved Hide resolved

emcelroy marked this pull request as ready for review September 30, 2024 15:48

emcelroy requested review from bgoldowsky and scytacki September 30, 2024 15:48

chore: move toolbar button component to shared location

c12a82e

emcelroy temporarily deployed to branches September 30, 2024 17:21 — with GitHub Actions Inactive

Merge branch 'master' into 185987214-drawing-tile-navigator-panel

8bb86f2

emcelroy temporarily deployed to branches September 30, 2024 17:44 — with GitHub Actions Inactive

Merge branch 'master' into 185987214-drawing-tile-navigator-panel

37cf963

emcelroy temporarily deployed to branches October 1, 2024 17:26 — with GitHub Actions Inactive

bgoldowsky requested changes Oct 1, 2024

View reviewed changes

scytacki removed their request for review October 2, 2024 13:07

emcelroy and others added 10 commits October 4, 2024 12:06

chore: refactor drawing tile tests

cdc1641

refactor: add navigatable tile model, save navigator position, use re…

dd41f05

…nder prop instead of cloneElement, comments

fix: drawing elements clipped by viewport boundaries

afe2666

update class users in place

0598cc9

This uses applySnapshot to update the class users in place. This will make code using these users more efficient since they can just observer the user properties for changes.

Switch to using the MST environment for the groups store

40f0c20

Original the MST environment was avoided because I was planning to switch this store to just be MobX. However the group store needs to be synced with the data from the database, and it is easier to do that using MST than MobX.

Sync the group membership

d7d950b

This uses applySnapshot to sync the changes from the DB. This way the existing objects are updated instead of creating new ones. I also missed a change in stores when committing the last set of changes.

switch authAndConnect to async await

c000b36

add support for removed users

670d5cd

We use a timestamp for when the class information is retrieved from the portal. And compare this to the timestamp in the groupUser to figure out if this groupUser is older than the class info or newer than the class info.

portal service object and refresh class info

25ab4e3

The portal service object provides a place to store portal info like JWTs, and methods that use and update this info. This new object is used to refresh the class info if necessary after updating the group info from Firebase.

scytacki and others added 7 commits October 4, 2024 12:06

hide users removed from the class

fa18fa6

kick removed users out of groups first

bac6d5b

add test of removed users

9fc990c

chore: update drawing test snapshots

d00a822

fix: increase wait for animation

d3f4af9

Merge branch 'master' into 185987214-drawing-tile-navigator-panel

2b8f966

chore: update non-legacy drawing test snapshots

1c688a6

emcelroy temporarily deployed to branches October 4, 2024 18:45 — with GitHub Actions Inactive

emcelroy requested a review from bgoldowsky October 4, 2024 19:05

refactor: simplify showing elements outside viewport

bf64e8a

emcelroy temporarily deployed to branches October 7, 2024 19:40 — with GitHub Actions Inactive

Merge branch 'master' into 185987214-drawing-tile-navigator-panel

58c7e68

emcelroy temporarily deployed to branches October 7, 2024 19:59 — with GitHub Actions Inactive

bgoldowsky requested changes Oct 8, 2024

View reviewed changes

emcelroy added 2 commits October 8, 2024 10:42

chore: remove plugin dependencies,

8f1ffcf

Merge branch 'master' into 185987214-drawing-tile-navigator-panel

953cef8

emcelroy temporarily deployed to branches October 8, 2024 14:49 — with GitHub Actions Inactive

emcelroy requested a review from bgoldowsky October 8, 2024 15:07

bgoldowsky reviewed Oct 8, 2024

View reviewed changes

chore: remove IDrawingTileProps import

e50c2a4

emcelroy deployed to branches October 8, 2024 15:36 — with GitHub Actions View deployment

bgoldowsky approved these changes Oct 8, 2024

View reviewed changes

emcelroy merged commit c078302 into master Oct 8, 2024
17 checks passed

emcelroy deleted the 185987214-drawing-tile-navigator-panel branch October 8, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: drawing tile navigator panel (PT-185987214) #2416

feat: drawing tile navigator panel (PT-185987214) #2416

emcelroy commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

cypress bot commented Sep 30, 2024 •

edited

Loading

bgoldowsky left a comment

emcelroy commented Oct 1, 2024

bgoldowsky left a comment

bgoldowsky Oct 4, 2024

emcelroy Oct 8, 2024

bgoldowsky Oct 4, 2024

emcelroy Oct 8, 2024

bgoldowsky Oct 8, 2024

bgoldowsky Oct 8, 2024

bgoldowsky Oct 8, 2024

bgoldowsky left a comment

		import { BoundingBox } from "../../plugins/drawing/model/drawing-basic-types";
		import { IDrawingTileProps } from "../../plugins/drawing/components/drawing-tile";

		@@ -0,0 +1,45 @@
		import { types, Instance, SnapshotIn } from "mobx-state-tree";

		import { BoundingBox } from "../../plugins/drawing/model/drawing-basic-types";

feat: drawing tile navigator panel (PT-185987214) #2416

feat: drawing tile navigator panel (PT-185987214) #2416

Conversation

emcelroy commented Sep 30, 2024 • edited Loading

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

cypress bot commented Sep 30, 2024 • edited Loading

bgoldowsky left a comment

Choose a reason for hiding this comment

emcelroy commented Oct 1, 2024

bgoldowsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgoldowsky left a comment

Choose a reason for hiding this comment

emcelroy commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

cypress bot commented Sep 30, 2024 •

edited

Loading