-
Notifications
You must be signed in to change notification settings - Fork 34
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 demo site with integrated capture and graph/download, plus Cypress tests #175
Conversation
Yechao change
Thanks for opening this pull request! |
examples/new-capture/index_copy.html
Outdated
@@ -0,0 +1,363 @@ | |||
<!DOCTYPE html> |
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.
Just wondering how this is distinct from index.html
? Thank you!
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.
This looks fantastic! A couple small notes and requests:
- I noticed a
.DS_Store
file was added, probably inadvertently - can you just remove that? Thanks! - The copy of the
new-capture
JS files seems like it could diverge development of those files, which may be changing quickly. Was it intentional to make 2 copies of each, and are they exact copies? - The
new-capture
index.html is still pointed atdist/capture.dist.js
, and that file is compiled from theexamples/capture/*.js
files, not the newnew-capture
copies, so i believe the new copies are not being used in this demo, is that correct? Below is the Gruntfile code which compiles the files:
spectral-workbench.js/Gruntfile.js
Lines 49 to 57 in 5b176c5
capture: { | |
src: [ | |
'examples/capture/detect_sample_row.js', | |
'examples/capture/toggleRotation.js', | |
'examples/capture/getRow.js', | |
'examples/capture/getUserMedia.js' | |
], | |
dest: 'dist/capture.dist.js' | |
} |
Thanks, just need a little clarity on these but overall this is looking really fantastic. I love your final report as well, would you be able to share that in this PR somehow for other contributors to read?
Great work, we're almost there!! Many thanks!
|
||
|
||
<script src="capture.js" type="text/javascript"></script> | ||
<script src="../../dist/capture.dist.js" type="text/javascript"></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.
This is pointed at the compiled file, see review note about Grunt compilation! Thanks!
I'm also seeing an odd error in our Travis build:
https://travis-ci.com/github/publiclab/spectral-workbench.js/builds/182860297#L399-L407 This seems related to the |
Ah, i am getting an error and no graph on the final "save graph" step: It looks like
Perhaps it has to be included via NPM instead of bower -- the correct route should be |
bower.json
Outdated
"moment": "~2.10.6", | ||
"nvd3": "~1.7.1", | ||
"d3": "~3.3.13", | ||
"jquery.steps": "^1.1.0" |
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.
Here I think we should move this to the package.json
file, as we are trying to stop using Bower. I believe all existing dependencies have already been moved, too!
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.
spectral-workbench.js/package.json
Line 27 in 5b176c5
"d3": "~3.5.17", |
examples/new-capture/index.html
Outdated
<script src="../../node_modules/flot/jquery.flot.crosshair.js"></script> | ||
<script src="../../node_modules/flot/jquery.flot.threshold.js"></script> | ||
<script src="../../node_modules/bootstrap/dist/js/bootstrap.min.js"></script> | ||
<script src="../../bower_components/jquery.steps/build/jquery.steps.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.
Yes - here, and below, all instances of bower_components
should be changed to node_modules
and all dependencies but jquery.steps
have been transferred there already. That should resolve the last error on the final "plotting" page of the new demo!
If you open the original index.html file, it has a hyper link to the new
demo site. Also please do another npm install and bower install before
running the new demo page. Thanks.
…On Thu, Sep 17, 2020, 12:37 PM Jeffrey Warren ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/new-capture/index_copy.html
<#175 (comment)>
:
> @@ -0,0 +1,363 @@
+<!DOCTYPE html>
Just wondering how this is distinct from index.html? Thank you!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#175 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEX5LH3NKMVLSEKY6JCM2N3SGI3MNANCNFSM4QZYQAIQ>
.
|
Something's wrong with d3... but in jasmine!
|
'bower_components/nvd3/build/nv.d3.js', | ||
'bower_components/bootstrap-css/js/bootstrap.min.js', | ||
'bower_components/moment/moment.js', | ||
'node_modules/d3/d3.js', |
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 something be wrong with this line, for jasmine?
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.
ah yes! I think d3 would be in a dist
folder... checking...
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.
Hmm. <script src="../../node_modules/d3/d3.js" type="text/javascript"></script>
indicates not...
Oh hmm, a bunch of Jasmine is broken:
|
OK, this is odd. But regarding:
That
Is there an order of execution issue happening? No, i think perhaps the SVG test failure is causing the |
OK, reproduced it by going to the graph example at /examples/index.html, copying graph2 = new SpectralWorkbench.Graph({
spectrum_id: 9,
calibrated: true,
onImageComplete: function() { done(); }
}); Here's the full trace:
|
OK - this is a known error: #165 re: d3 v3.5 vs. 3.4. Don't know why it's just suddenly showing up on Travis. Let's downgrade again and see. |
Trying to bump d3 to graph demo works: new capture demo works: I think we could do this... but will |
Aha ok, so: noting and, back to the original error:
|
OK so we have trouble with d3 v3.5 - the known error in our codebase: #165 But v3.4 had caused us trouble with So we are left with:
|
So, noting that this error would occur a LOT in this file:
because this segment causing it is repeatedly called in that file: var id = d3.select('svg').data()[0][index].id; // this is the real d3 DOM-stored data The error cited above was for the first instance. So, we can dig into exactly what is not an object but probably there's something wrong with either the DOM or how we're accessing it. Actually given |
Noting the error occurs in client side of the graph demo! whoa! OK, here are the values: d3.select('svg').data()[0]
> (4) [{…}, {…}, {…}, {…}]
0: {values: Array(21), key: "undefined (average)", color: "#444", id: undefined}
1: {values: Array(21), key: "undefined (R)", color: "rgba(255,0,0,0.2)"}
2: {values: Array(21), key: "undefined (G)", color: "rgba(0,255,0,0.2)"}
3: {values: Array(21), key: "undefined (B)", color: "rgba(0,0,255,0.2)"}
length: 4__proto__: Array(0)
d3.select('svg').data()[0][0].id
> undefined Something must be going wrong here while inserting this data: spectral-workbench.js/src/SpectralWorkbench.Graph.js Lines 218 to 220 in 7b52a77
OK, yes, it's when we call graph.datum.d3()
> (4) [{…}, {…}, {…}, {…}]
0: {values: Array(21), key: "undefined (average)", color: "#444", id: undefined}
1: {values: Array(21), key: "undefined (R)", color: "rgba(255,0,0,0.2)"}
2: {values: Array(21), key: "undefined (G)", color: "rgba(0,255,0,0.2)"}
3: {values: Array(21), key: "undefined (B)", color: "rgba(0,0,255,0.2)"} |
Hmm, maybe the two errors are different, |
Solved Now we have one more error -- all these are returning incorrectly... could be related:
spectral-workbench.js/spec/javascripts/set_spec.js Lines 54 to 63 in 7b52a77
|
No more test failures! Just:
|
Related to node 10, switched to 12! gruntjs/grunt-contrib-jasmine#266 |
YES!!!! All done! Merging! Thank you!!!! |
Congrats on merging your first pull request! 🙌🎉⚡️ |
Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!
Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software
Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.
Thanks!