-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/#122 rescale window #157
Conversation
Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
…usted in next commit Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
One more comment, I removed the Log for this version, if we want it back I'll add it again. |
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
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!
Awesome! I think the background could need some kind of color shades or some separation of control units but that's something for another ticket. |
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.
Also works in Safari
* | ||
* @param {MessageEvent} messageEvent - has poperty (Float64Array) data | ||
*/ | ||
const socketOnMessage = (messageEvent) => { |
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.
Feels wrong to have the websocket and update buffer logic in a file named Layout. Not a big deal as it wasn't much better before.
Git didn't pick up that this was a rename from Oscilloscope.svelte -> Layout.svelte.
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.
Git didn't pick up on it cause it wasn't ;)
I created Layout.svelte
separately to build the whole Layout from scratch so I don't have to mingle with the existing one. Will rename to Oscilloscope.svelte
, was a good point.
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 really like the design. Looks like quite a bit of work. Kind of hard to review because it is so large. Tested the UI and feels good!
Currently working on your and Mr. Eckert's recommendations, will hopefully be able to commit those changes later today. Else we'll simply keep the issue open for the next sprint and conclude it then. |
…h mobile and laptop screens Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
Sorry in advance for the humongous PR ;)
Works on Chromium and Firefox, @Mac guys, I'd very much appreciate it if you'd check if it works on Safari as well.