-
Notifications
You must be signed in to change notification settings - Fork 22
Add timestamp #947
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
base: master
Are you sure you want to change the base?
Add timestamp #947
Conversation
Snacj
left a comment
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 address the comments and see if timestamps work on actual machines.
| // Assuming the graph's fixed Y-scale is from -1 to 1 based on the sine wave example | ||
| const graphMin = -1; | ||
| const graphMax = 1; | ||
| // TODO: For real-world graphs (like Winder), you might need to read the actual min/max scale |
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.
We can't assume that the min/max of the y axis will always be -1 and 1.
See roundness value of the laser (min: 0, max: 100)
|
|
||
| const graphEl = graphWrapperRef.current; | ||
| // The BigGraph component is the first child (the one with the actual chart) | ||
| // TODO: Find a better way to do this |
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.
If there is a better way to do it, please implement. Otherwise delete the comment.
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 test the Graph on that machine. It may not work due to the y-axis limitation.
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 test the Graph on that machine. It may not work due to the y-axis limitation.
|
@Snacj Please finish up this Issue asap, bach's internship is over |
|
@Snacj Could you do a review? I've fixed the issues. |
TheBest6337
left a comment
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.
When I add an marker and move to another page or machine it isnt saved.
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.
Why is the size and position of the marker changing? It should rather be always at the same height in the graph window. The Text also seems to be above the value window which doesnt look right, because of the shadow it would look better if it is under it. The red line is also weird, is it an bug or a feature? It appears after adding one marker and I dont understand what it does...
|
Im also not sure if i am a fan of the many "Add Marker" buttons, we could maybe just do one button that creates an marker for the whole machine, this could just be next to the Export button for example. Then when clicked an popup opens where you can specify the time (if wanted) and add an name or color. And also if i click on the marker button i expect that the marker appears at the current live time, at the moment it comes after a few seconds. So when using the popup just always use the current time. |
Able to add markers for timestamps and export in a new excel file. Can still improve with the code. The real scale from the Y axis of the graph is not done, right now only hard code.