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

Bar graph sync #2404

Merged
merged 7 commits into from
Sep 17, 2024
Merged

Conversation

bgoldowsky
Copy link
Contributor

@bgoldowsky bgoldowsky commented Sep 13, 2024

PT-188129121

This PR fixes some issues noted in the original bar graph work.

  • The way it finds and tracks changes to its shared model is updated so that all views should properly update.
  • Y axis name updates now propagate to read-only views.
  • All editing operations are correctly disabled in read-only views.
  • Duplicating a bar graph tile should now work since there is a function to update the IDs on copy.

A lot more tests have been added, including checking that the display in the read-only views is correct after each change (this requiring some updates to the standalone editor HTML and TableToolTile.js so that each view could ba accurately targeted by Cypress).

Also logging has been added.

Add ID update function to fix tile copy issue.
Prevent changes in read-only mode.
When changing primary attribute, reset secondary as part of the same action to support undo.
Make EditableAxisLabel handle things more locally to fix sync bugs.
Test read-only views
Test undo and redo of each operation.
Add classes in DocEditorApp to support testing.
Copy link

cypress bot commented Sep 13, 2024

collaborative-learning    Run #13752

Run Properties:  status check passed Passed #13752  •  git commit f32d8144a2: More tests
Project collaborative-learning
Branch Review 188129121-bar-graph-sync
Run status status check passed Passed #13752
Run duration 14m 51s
Commit git commit f32d8144a2: More tests
Committer Boris Goldowsky
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 6
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 111
View all changes introduced in this branch ↗︎

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 94.02985% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.98%. Comparing base (d8acf33) to head (f32d814).
Report is 9 commits behind head on 188127118-bar-graph-link.

Files with missing lines Patch % Lines
src/plugins/bar-graph/bar-graph-content.ts 90.00% 2 Missing ⚠️
src/plugins/bar-graph/editable-axis-label.tsx 93.33% 1 Missing ⚠️
src/plugins/bar-graph/legend-area.tsx 92.85% 1 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##           188127118-bar-graph-link    #2404      +/-   ##
============================================================
+ Coverage                     85.93%   85.98%   +0.05%     
============================================================
  Files                           741      741              
  Lines                         37981    37996      +15     
  Branches                       9688     9689       +1     
============================================================
+ Hits                          32640    32672      +32     
+ Misses                         5035     5019      -16     
+ Partials                        306      305       -1     
Flag Coverage Δ
cypress-regression 77.58% <93.22%> (+0.05%) ⬆️
cypress-smoke 27.94% <3.44%> (-0.02%) ⬇️
jest 48.71% <62.50%> (+0.04%) ⬆️

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.

Base automatically changed from 185294648-bar-graph-legend to 188127118-bar-graph-link September 16, 2024 14:44
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.

This looks great to me. All of the Cypress tests are excellent.

@bgoldowsky bgoldowsky merged commit cb197c0 into 188127118-bar-graph-link Sep 17, 2024
17 checks passed
@bgoldowsky bgoldowsky deleted the 188129121-bar-graph-sync branch September 17, 2024 17:05
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