Skip to content
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

Implemented feature #62-cursor #114

Merged
merged 5 commits into from
Dec 7, 2022
Merged

Implemented feature #62-cursor #114

merged 5 commits into from
Dec 7, 2022

Conversation

motschel123
Copy link
Collaborator

Implemented the cursor by injecting undefined data (not being displayed) in front of the new sample data.

image

Signed-off-by: Marcel Schöckel marcel.schoeckel@gmail.com

Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
@motschel123 motschel123 linked an issue Dec 3, 2022 that may be closed by this pull request
14 tasks
@cypress
Copy link

cypress bot commented Dec 3, 2022



Test summary

8 0 0 0


Run details

Project sosci-frontend
Status Passed
Commit 8cec827
Started Dec 7, 2022 8:56 AM
Ended Dec 7, 2022 8:57 AM
Duration 01:10 💡
OS Linux Ubuntu - 22.04
Browser Chrome 107

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

@@ -34,3 +34,4 @@ export const MIN_SWEEP = 0.5; // <= 1
export const MAX_SWEEP = 2.0; // >= 1
export const MIN_AMPLITUDE = 0.0;
export const MAX_AMPLITUDE = NUM_INTERVALS_HORIZONTAL / 2;
export const WAVE_CURSOR_SIZE = 50;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename to WAVE_CURSOR_WIDTH, would be more in line with the rest of the code :)

@@ -103,9 +122,9 @@
};

const update = () => {
for (let i = 0; i < channel_samples.length; i++) {
for (let i = 0; i < channelSamplesInjectCursor.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not happy with an extra for loop only to "write" the cursor, currently looking for a way to fix this, will let you know if I find a better solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the old loop, because the update function doesn't know where we currently are, it's just plots all the data.
So I first inject the cursor when data arrives.

Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
Copy link
Collaborator

@PhlppKrmr PhlppKrmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed my suggested changes now. Let me know if it is still fine with you.

Copy link
Collaborator

@PhlppKrmr PhlppKrmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests missing.

@motschel123
Copy link
Collaborator Author

If you know a way to test it please let me know. It's the same as the basic waves, we can't really test for web gl rendering or the correct data injection from inside cypress.

@PhlppKrmr
Copy link
Collaborator

In #111 we had a similar issue where we don't quite know how to test plotting functionalities. I'd suggest we open up an issue that evaluates how to test those additionally to the frontend testing issue we still have in the sprint backlog. Are you fine with that?

jenswaechtler and others added 2 commits December 7, 2022 09:49
# Conflicts:
#	Apps/frontend/src/components/Waves.svelte
Co-authored-by: Philipp Kramer <philipp.kramer@fau.de>
Co-authored-by: Leander Tolksdorf <leandet98@zedat.fu-berlin.de>
Co-authored-by: Jan Degen <jan.degen@fau.de>
Signed-off-by: Jens Wächtler <jens.f.waechtler@fau.de>
@jandegen jandegen merged commit eb506eb into dev Dec 7, 2022
@jandegen jandegen mentioned this pull request Dec 7, 2022
@jandegen jandegen deleted the feature/62-cursor branch December 7, 2022 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor
6 participants