-
Notifications
You must be signed in to change notification settings - Fork 137
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
Preference test #91
base: master
Are you sure you want to change the base?
Preference test #91
Conversation
…version less than 8 because it was removed.
… show a confirmation code on the final page and to generate a random subject ID for each session
…ouped.yaml more different than the pref.yaml
@nullpunktTUD this is awesome. Thanks! I won't have time to review this before end of June. There are some javascript linter that could detect some errors maybe run this before... |
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.
As I said, I won't be able to provide a full review but this looks already quite good to me.
@nullpunktTUD is there things missing? Otherwise I would give this a test and if things work as advertised, we can merge this.
Please also see my minor comments
this.filePlayer.genericAudioControl.addEventListener((function (_event) { | ||
if (_event.name == this.pageConfig.mustPlayback) { | ||
this.played[_event.index] = true; | ||
if (this.played.every(element => element === true)){ |
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.
Not sure, but I think the use of =>
is incorrect 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.
if (this.played.every(element => element === true)){ | |
if (this.played.every((element) => element === true)){ |
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 this code was written by me. I'm happy to make the change, if you want me to, though. Please confirm.
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.
Yes, go a head. I discovered this using the grunt package
task. You can check yourself if this solves the error.
I can try to enable this on github action in the future
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.
@nullpunktTUD i realized this is ES6 syntax, are you able to find and alternative that works on older browsers?
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.
Ok. I will look into this later
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.
Thanks!
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.
Can you find an alternative for 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.
@Simon-Stone do you have an update on this? i'm not a js expert myself but maybe chatgpt could help 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 have an update, no. I can try to petition our AI overlords, though, and report back
Thank you for taking the time to consider the PR! I don't think there is anything missing and it seems to be working just fine. I'll try to resolve all your minor comments soon. |
Co-authored-by: Fabian-Robert Stöter <[email protected]>
Are there any changes left for me to make? |
Will look at this again end of the week |
this would be a super useful PR 😄 @nullpunktTUD Looks like you should add the pref test to |
DISCLAIMER: I am by no means a JavaScript programmer and I have cannibalized various other pages to clobber the new page together. I am absolutely certain that there the code could use some refactoring and cleanup, but I unfortunately do not have access to any experienced JavaScript programmers. If someone would take the time to do a proper code review of this PR, I would be happy to make any required changes, though.
I have a particular use case that was not covered by the current framework:
I want to compare a list different stimuli amongst itself in pairwise fashion. There is no reference or baseline, I simply want to compare every stimulus against every other stimulus and ask the subject which one they prefer. I also want to run the experiment online, possibly using crowdsourced participants, which means I need a way to discern who actually completed the experiment.
This PR adds several things to support this use case:
preference_test
,All new features are documented in
experimenter.md
andparticipant.md
.Two new YAML files
pref.yaml
andpref_grouped.yaml
are added toconfigs/
to illustrate the use of the new page and options.Closes #88