-
Notifications
You must be signed in to change notification settings - Fork 1
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
Stg/graph #6
base: master
Are you sure you want to change the base?
Conversation
For review puproses, I left the full current graph content in the index html to see the full purpose of it all. Will also add a documentation for the component, once done and clear with RBB. |
src/xrdok.js
Outdated
|
||
// create secondary vertices | ||
planeGraphSecondaryGeo.vertices.push( | ||
new THREE.Vector3(-sec, prim + 0.001, -j), |
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.
Might even need a 0.002 offset, since there is a chance of flickering still to be seen.
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.
@HelrenPDM let me know if you have questions with my review comments
function createData(data, el) { | ||
// parse CSV, if available | ||
if (data.src) { | ||
Papa.parse(data.src, { // eslint-disable-line no-undef |
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 specify Papa in globals
section of eslint.rc. then you can remove the comment line here
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 don't think we have an eslint.rc checked in yet, right?
|
||
createData(data, el); | ||
|
||
this.lineEl = document.getElementById('line-graph-parent'); |
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.
took me a while to understand what you are doing here..:)
it would be much easier to understand if you just return these values from createData
function like you did in all the other create functions.
Also I think there is a bug here. Since Papa.parse
complete
callback can be called after you make the assignments here. Meaning you could get null values in your members as the referenced dom elements are just not there yet.
You need to assign these members after Papa.parse
completes.
update: function(oldData) { | ||
var data = this.data; | ||
|
||
const lineEl = document.getElementById('line-graph-parent'); |
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.
Thats why you have to do the assignments again here. Because the elements are just not there on the first run.
Yeah, still some work to do with this. |
…ity property dependent
Will still clean this up, only updated again to get the gaze curser enabled for the showcase. |
See https://glitch.com/~climate-to-the-sun or https://climate-to-the-sun.glitch.me/
Added an xr-plane-graph component to show 1 or 2 dimensional csv data. Partly WIP (line-graph)