-
Notifications
You must be signed in to change notification settings - Fork 19
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
New module ConsultDoc #13
Conversation
Some GUI related comments first:
A few other things:
I will look deeper in the code after these modifications are ok :) |
About commit 32a66af : Research: |
|
UDV-Core/examples/Demo.js
Outdated
|
||
var extendedDocumentModel = { |
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 think this information should be stored in a config file (possibly shared by server side software components ?). Is this planned for another pull request ?
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.
Information is now stored in examples since it runs for a demonstration. I am not sure it should be shared by server side software components.
UDV-Core/examples/Demo.js
Outdated
documentController = datDotGUI.add( documents, 'docBrowserWindowIsActive' | ||
).name( "Documents" ).listen(); | ||
documentController.onFinishChange( function(value) { documents.toggleDocBrowserWindow(); }); | ||
var billboardOption = documentFolder.add( controller.documentBillboard, 'windowIsActive').name("Billboard(soon)").listen(); |
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.
Please remove this button for the time being as billboard representation of documents has been postponed to a new pull request.
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.
Done
'; | ||
|
||
var optionsFilter = "http://rict.liris.cnrs.fr/optionsFilter.json"; | ||
var schema = "http://rict.liris.cnrs.fr/schemaType.json"; |
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.
The number of configuration files is growing up, I think that it could be a good idea to list them (along with their roles) and to provide examples in a dedicated page pointed in the README.md.
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.
The Readme.md is in UDV-Core/src/Modules/ConsultDoc
UDV-Core/webpack.config.js
Outdated
@@ -22,6 +22,10 @@ module.exports = { | |||
plugins: [ | |||
definePlugin, | |||
new webpack.optimize.CommonsChunkPlugin({ name: 'udvcore' }), | |||
new webpack.ProvidePlugin({ |
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.
Please add a comment saying that this is related to alpaca
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.
done
UDV-Core/webpack.config.js
Outdated
@@ -62,4 +66,9 @@ module.exports = { | |||
devServer: { | |||
publicPath: '/dist/', | |||
}, | |||
resolve: { |
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.
Could you add a comment saying that this is related to alpaca.
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.
done
General remark: This is a first step towards a MVC architecture as the code from the views, the controller and the model are separated in different files for the ConsultDoc module. However the next step to reach a MVC architecture would be that the views and the model don't know about the controller (which is currently not the case). A way to manage this would be by using events triggered by the views and the model and trapped by the controller. The step after that would be to generalize this approach to the other modules (e.g. temporal) and between modules. This work is postponed to other pull requests and I will merge this one when the remaining asked modifications are done :) |
Two remarks:
|
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.
Nice work ! We are almost there for this PR.
UDV-Core/examples/Demo.html
Outdated
@@ -13,6 +13,7 @@ | |||
<script src="../node_modules/moment/moment.js"></script> | |||
<script src="../node_modules/three/build/three.js"></script> | |||
<script src="../src/Externals/dat.gui/dat.gui.min.js"></script> | |||
<script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.0/jquery.min.js"></script> |
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.
The dependency on Jquery is already expressed (with this PR) in package.json.
The import of Jquery thus doesn't need to depend on on an external server (ajax.googleapis.com) to be active.
Use the "classic" import mechanism to remove that URL (and the dependency towards an active external server as well as the need for IP connectivity).
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.
Created issue #15
UDV-Core/examples/Demo.js
Outdated
@@ -125,9 +125,42 @@ var help = new udvcore.HelpWindow({active:true}); | |||
// FIXME For the time being this demo uses the Vilo3D data. Provide a | |||
// default document for the demo of DocumentHandler class and place it | |||
// within src/Modules/Documents... |
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.
Remove this comment that was associated with the following documents variable (and the Vilo3D/docs.csv data file).
Instead provide a new comment introducing the DocumentController. In particular explain why the DocumentModel, the researchModel (and OptionsResearch) appear at this level (why place it here and not "inisde" the DocumentController). This will take a couple lines of explanation...
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.
done
UDV-Core/examples/Demo.js
Outdated
docResearch.onFinishChange(function(value){ | ||
controller.documentResearch.refresh(); | ||
}); | ||
/* |
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.
Remove the following commented out code (that will come in an upcoming different PR).
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.
done
UDV-Core/examples/DocumentModel.json
Outdated
@@ -0,0 +1,82 @@ | |||
{ |
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.
Would it be possible to regroup DocumentModel.json, optionsFilter.json, and schemaFilters.json within an examples/Document (or Contribute?) sub-directory to indicate they work together ?
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.
For the general demo (with all options / functionnalities), the files are in UDV-Core/examples/data
For the demo regarding consultDoc module, they are in: UDV-Core/src/Modules/ConsultDoc/examples
UDV-Core/examples/Demo.js
Outdated
documentController.onFinishChange( function(value) { documents.toggleDocBrowserWindow(); }); | ||
//Document uses a folder | ||
var documentFolder = datDotGUI.addFolder("Documents"); | ||
var docResearch = documentFolder.add( controller.documentResearch, 'windowIsActive').name("Research").listen(); |
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.
Respect the (implicit) coding style and the 80 lines wide constraint.
Manually break (newlines) the lines to make the code more readable !
@@ -0,0 +1,273 @@ | |||
|
|||
import * as THREE from 'three'; |
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.
Same remark as for Demo.js concerning the lines longer than 80 columns.
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.
done
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.
Well, many lines still look much longer than 80 columns to me. Am I wrong ?
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.
Fixed
{ | ||
|
||
this.url = "http://rict.liris.cnrs.fr/APIVilo3D/APIExtendedDocument/web/"; //url of the server handling documents | ||
//FIXME: to put in a configuration file of the general application |
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 just created issue #14 to get this fixed in the next PR.
Issues are much more "sticky" (better reminders) than FIXMEs hidden in the code.
…mo. Code made more readable (breaking lines). Fixed image size in browser
Good job; we did it! :) |
Functionnality research, browser OK
Billboard : work in progress