-
Notifications
You must be signed in to change notification settings - Fork 41
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
Export game view #1690
Export game view #1690
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1690 +/- ##
==========================================
- Coverage 85.89% 85.82% -0.08%
==========================================
Files 607 607
Lines 31133 31154 +21
Branches 8665 8663 -2
==========================================
- Hits 26743 26739 -4
- Misses 4068 4091 +23
- Partials 322 324 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
codap-v3 Run #5616
Run Properties:
|
Project |
codap-v3
|
Branch Review |
main
|
Run status |
Passed #5616
|
Run duration | 06m 01s |
Commit |
35b7376c27: Merge pull request #1690 from concord-consortium/188616810-export-gameview
|
Committer | Kirk Swenson |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
1
|
Pending |
36
|
Skipped |
0
|
Passing |
220
|
View all changes introduced in this branch ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good -- I added a commit which fixed the game view name
issue you pointed out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two thoughts occurred to me this morning:
- Instead of a Boolean
isPlugin
property, it should be asubType
property with valueplugin
. That way whenDG.GuideView
is implemented, it can besubType
guide
. I would leave it undefined for standard web views. - The importer for
DG.GameView
should set thesubType
property toplugin
.
After consultation with @scytacki, I added a commit which implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After consultation with @scytacki, I addressed my concerns in a follow-up commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't officially approve but it looks good to me.
My only comment is about allowing the v3 WebView to be turned back into a v2 WebView via the subType setter.
Good catch -- I meant to do that. I'll update before merging. |
This makes `isPlugin` a serialized field in V3. That is necessary so we can keep track of which WebViews are v2 GameViews and which are v2 WebViews. The `setIsPlugin` is marked as `withoutUndo` so when a plugin is added and it sets up the iframe phone this change of the document state doesn't add an action to the undo stack. The `contextStorage` field is added to the DataSet conversion. This was necessary to get a document to load with a plugin that had created a data table. However this document is still broken because it data context collections seem to be reversed and disconnected. Name handling in GameViews adds more complexity to the already complex name and title handling. GameViews store their name in componentStorage.currentGameName instead of componentStorage.gameName
a2c0015
to
ad52a2d
Compare
This makes
isPlugin
a serialized field in V3. That is necessary so we can keep track of which WebViews are v2 GameViews and which are v2 WebViews.The
setIsPlugin
is marked aswithoutUndo
so when a plugin is added and it sets up the iframe phone this change of the document state doesn't add an action to the undo stack.The
contextStorage
field is added to the DataSet conversion. This was necessary to get a document to load with a plugin that had created a data table. However this document is still broken because it data context collections seem to be reversed and disconnected.Name handling in GameViews adds more complexity to the already complex name and title handling. GameViews store their name in componentStorage.currentGameName instead of componentStorage.name