bar graph link #2398
bar graph link #2398
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2398 +/- ##
==========================================
+ Coverage 86.16% 86.29% +0.13%
==========================================
Files 736 739 +3
Lines 37814 37955 +141
Branches 9626 9680 +54
==========================================
+ Hits 32584 32755 +171
+ Misses 4931 4903 -28
+ Partials 299 297 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
collaborative-learning
|
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
188127118-bar-graph-link
|
| Run status |
|
| Run duration | 13m 40s |
| Commit |
|
| Committer | Boris Goldowsky |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
3
|
Skipped
|
0
|
Passing
|
110
|
| View all changes introduced in this branch ↗︎ | |
Restyle 'vertical' layout
There was a problem hiding this comment.
I'm going to approve this even though I found a couple of problems. I figure you'd likely want to fix those in a later PRs.
When I duplicated a bar graph tile the document got corrupted:
https://collaborative-learning.concord.org/branch/bar-graph-link/?appMode=demo&demoName=ScottRemote&fakeClass=1&fakeUser=student:1&problem=0.1&unit=./demo/units/qa/content.json
I also noted where it seems the dataSet should be a view instead of a volatile. I know there is a separate story about fixing the syncing, so that doesn't need to be addressed here.
And perhaps the document crash is also something you want to punt to another PR.
| if (self.dataSet !== dataSet) { | ||
| self.setPrimaryAttribute(undefined); | ||
| self.setSecondaryAttribute(undefined); | ||
| self.dataSet = dataSet; |
There was a problem hiding this comment.
This is where it'd be better to have a get dataSet view that is using MST to cache the dataSet instead of doing it manually here in the updateAfterSharedModelChanges action. Many other tile content models use a similar pattern. If you search for get sharedModel() I think you'll find them all.
|
I see. The crash is due to a simple omission of a null check. But it reveals that I neglected to provide an |
|
Just noting that duplicating the tile will also make a copy of the dataset - the new tile will not remain connected to whatever other tile the original bar graph was linked to. This seems to be how all CLUE tile duplicates work. |
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.
…legend Bar graph responsive styling
…sync Bar graph sync
This includes all of PT-188127118 and PT-188212456, and parts of PT-185294648 .
Toolbar added; single button to link/unlink datasets.
By default the first attribute from dataset is set as the primary category for the bar graph; this can be changed with the x-axis pulldown menu.
By default there is no secondary ("Sort by") attribute, but one can be chosen from the legend.
Bar graph should be displayed properly with no data, single attribute, or two attributes.
Changes to dataset should be immediately reflected, including naming, attributes added/deleted/changed, values changed, cases added/removed.
A later PR will round out the functionality for the legend, including automatically adjusting tile height and improved design fidelity.