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

Feature/#117 distribute button #148

Merged
merged 13 commits into from
Jan 18, 2023
Merged

Conversation

jenswaechtler
Copy link
Contributor

Added a button for distributing signals evenly on the canvas

image
Image of three signals after clicking the button

@cypress
Copy link

cypress bot commented Dec 20, 2022



Test summary

33 0 0 0


Run details

Project sosci-frontend
Status Passed
Commit c81ea5d
Started Jan 18, 2023 8:34 AM
Ended Jan 18, 2023 8:36 AM
Duration 02:14 💡
OS Linux Ubuntu - 22.04
Browser Chrome 108

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

Copy link
Contributor

@nicolaskolbenschlag nicolaskolbenschlag left a comment

Choose a reason for hiding this comment

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

Screenshot from 2022-12-20 20-19-05

I tried it. If I refresh the page, deactivate any instead of the first two signals, turn on and then click distribute, the plot looks like in the screenshot. The signals are distributed but so as they would be as if all 10 channels were active. Maybe this is a user-mistake from me. Could you please provide more information on how to test this?

Furthermore, you screenshot indicates that signal one (red) is at the bottom. I would prefer to maintain the order of the signals as used in the sliders.

@jenswaechtler
Copy link
Contributor Author

jenswaechtler commented Dec 21, 2022

Screenshot from 2022-12-20 20-19-05

I tried it. If I refresh the page, deactivate any instead of the first two signals, turn on and then click distribute, the plot looks like in the screenshot. The signals are distributed but so as they would be as if all 10 channels were active. Maybe this is a user-mistake from me. Could you please provide more information on how to test this?

Furthermore, you screenshot indicates that signal one (red) is at the bottom. I would prefer to maintain the order of the signals as used in the

I always distribute all 10 channels.
The description says to distribute all channels that are plotted. The channels that are turned off with the On Off button are still displayed and not removed from the plot. What we would need is a filter for the different channels to hide them completely. Then they should not be distributed anymore. So far we don't have that function.
For the screenshot, I changed the number of channels in the code to 3, because it gets very confusing with 10 channels.

I can change the arrangement of the channels.

@motschel123
Copy link
Collaborator

If I remember correct meeting right we should have all channels off on start per default and only disturbute channels that are switched on.
I'll try and implement that.

Copy link
Contributor

@nicolaskolbenschlag nicolaskolbenschlag left a comment

Choose a reason for hiding this comment

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

Screenshot from 2023-01-09 07-50-03

I clicked distribute, then switched off any channel except first and second, then clicked distribute again. The channels did not change then.

Further, I would prefer to maintain the order of the channels as given with the sliders. Currently it is reversed.

@motschel123
Copy link
Collaborator

The distribution should only happen once on button click now.
I'll try and refactor the distribution code slightly as it's not that easy to understand (e.g. offset is being calculated multiple times)

@motschel123
Copy link
Collaborator

I move the offset data into a store (Array<Writable> for performance), but svelte is not able to access it ( $offsetStore[index] won't work ). I did make it work within docker build but then npm run build for cypress fails.
So I just reverted and kept it as It was.

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.

For me the waves aren't even displayed. Tried out the #122 commit and there everything works fine?
image

This means I sadly can't test out the functionality, code-wise it looks like it'd work.
Additionally, two more things:

  1. Can we please merge master into this branch so we actually use the new layout?
  2. Can you add some JSDoc for the functions you added?

Apps/frontend/src/components/DistributeOffsetButton.svelte Outdated Show resolved Hide resolved
Apps/frontend/src/components/DistributeOffsetButton.svelte Outdated Show resolved Hide resolved
Apps/frontend/src/components/WaveControls.svelte Outdated Show resolved Hide resolved
Apps/frontend/src/components/Waves.svelte Outdated Show resolved Hide resolved
Apps/frontend/src/components/WaveControls.svelte Outdated Show resolved Hide resolved
Apps/frontend/src/components/WaveControls.svelte Outdated Show resolved Hide resolved
Apps/frontend/src/components/WaveControls.svelte Outdated Show resolved Hide resolved
Apps/frontend/src/components/WaveControls.svelte Outdated Show resolved Hide resolved
Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
Copy link
Contributor

@nicolaskolbenschlag nicolaskolbenschlag left a comment

Choose a reason for hiding this comment

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

Looks good. But, when the button is clicked once, it distributes also when channels are (de-)activated. This is nice on one side and could be considered as a feature but I think it also has a drawback: namely when a custom offset is chosen via the slider for channel A and then channel B is activated or deactivated, the removes the user's custom offset of A. Maybe we should just distribute on-click and not always after the button is clicked once.
If this behavior is for purpose, it is also fine for me.

@motschel123
Copy link
Collaborator

@nicolaskolbenschlag can you elaborate on your comment? For me it only changes the offsets on channels that are switched on

Copy link
Collaborator

@leandertolksdorf leandertolksdorf left a comment

Choose a reason for hiding this comment

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

Whoops, just noticed the same thing as Philipp: In the indicators, the offsets should be pulled from the store like the amplitudes.

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>
@PhlppKrmr
Copy link
Collaborator

Werid artifact on indicators as can be seen in video below. Happens when I reload the page and as the first action press "Turn on" (no Channels started).

2023-01-17 19-00-11

Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
@PhlppKrmr
Copy link
Collaborator

PhlppKrmr commented Jan 17, 2023

Signal seems oddly inconsistent (x-axis).
Edit: Checked if it also happens in #122 for me, it does. Seems to be a local problem, will investigate some more.

image

motschel123 and others added 2 commits January 17, 2023 20:03
…e-button

Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
…t for distribute button

Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
…ate system

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

@motschel123 You missed some deprecated files in the merge, should be fixed now. Also changed text to icon, added the test I mentioned in one of my previous comments and improved scalability of UI. Should be good to go now.

@PhlppKrmr
Copy link
Collaborator

PhlppKrmr commented Jan 17, 2023

Signal seems oddly inconsistent (x-axis). Edit: Checked if it also happens in #122 for me, it does. Seems to be a local problem, will investigate some more.

image

One more thing, does this also happen on any of you guys' machines or is this a purely local problem on my side?

Signed-off-by: Philipp Kramer <philipp.p.kramer@gmail.com>
…eset-button-and-text-box' into feature/#117-distribute-button

Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
…eset-button-and-text-box' into feature/#117-distribute-button Part2

Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
Signed-off-by: Marcel Schöckel <marcel.schoeckel@gmail.com>
@motschel123 motschel123 dismissed stale reviews from leandertolksdorf and nicolaskolbenschlag January 18, 2023 08:33

Has been implemented

@motschel123 motschel123 merged commit 51d4e34 into dev Jan 18, 2023
@motschel123 motschel123 mentioned this pull request Jan 18, 2023
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.

6 participants