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

Added video several keyboard responses plugin #83

Merged
merged 6 commits into from
Dec 20, 2023
Merged

Added video several keyboard responses plugin #83

merged 6 commits into from
Dec 20, 2023

Conversation

marianylund
Copy link
Contributor

@marianylund marianylund commented Dec 12, 2023

Hello, this is still my first time contributing to an open-source project, so I will highly appreciate any feedback! Thank you @jodeleeuw for the suggestion to add a new plugin instead of changing the original plugin-video-keyboard-response.

Currently the jsPsych plugin-video-keyboard-response plugin records only one response and saves response time information and which key was pressed. I need to be able to play a video and record several inputs while user is watching the video and map it to the current time of the video to know when during the video user responded.

My plan was to continue with a very simple change on top of the existing plugin, so in the future it will be easier to merge. So, I´ve added a new boolean multiple_responses_allowed to control if only one response or multiple will be recorded (default true). Also added video_time to the data that is returned from the trial.

Copy link

changeset-bot bot commented Dec 12, 2023

🦋 Changeset detected

Latest commit: 366e2d0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@jspsych/plugin-video-several-keyboard-responses Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Shaobin-Jiang
Copy link
Contributor

The test result made it quite clear:

FAIL plugin-video-several-keyboard-responses packages/plugin-video-several-keyboard-responses/src/index.spec.ts
  ● video-several-keyboard-responses simulation › visual mode works

    expect(received).toBeGreaterThan(expected)

    Matcher error: received value must be a number or bigint

    Received has type:  array
    Received has value: [560]

      72 |     await expectFinished();
      [73](https://github.com/jspsych/jspsych-contrib/actions/runs/7180394352/job/19558500296?pr=83#step:7:74) |
    > [74](https://github.com/jspsych/jspsych-contrib/actions/runs/7180394352/job/19558500296?pr=83#step:7:75) |     expect(getData().values()[0].rt).toBeGreaterThan(0);
         |                                      ^
      [75](https://github.com/jspsych/jspsych-contrib/actions/runs/7180394352/job/19558500296?pr=83#step:7:76) |     expect(typeof getData().values()[0].response).toBe("string");
      [76](https://github.com/jspsych/jspsych-contrib/actions/runs/7180394352/job/19558500296?pr=83#step:7:77) |   });
      [77](https://github.com/jspsych/jspsych-contrib/actions/runs/7180394352/job/19558500296?pr=83#step:7:78) | });

      at Object.<anonymous> (src/index.spec.ts:74:38)

The original plugin used this line:

expect(getData().values()[0].rt).toBeGreaterThan(0);

Because the rt it records is a numeric value and since toBeGreaterThan expects a number or bigint, it worked fine. With your modified version, what is recorded is an array of rt, which is why the error thrown includes:

Matcher error: received value must be a number or bigint

    Received has type:  array
    Received has value: [560]

You should probably modify this part of the test to loop through the array or check upon the first element of the array or something of the sort.

@marianylund
Copy link
Contributor Author

Thank you so much, Shaobin-Jiang, I think I was able to fix it now

@bjoluc
Copy link
Member

bjoluc commented Dec 18, 2023

Good job on blaming the build failure on the dependencies @marianylund! @jspsych/config version 2 doesn't work with outdated Node.js versions and the CI for this repo still uses Node.js 14 and 16 instead of 18 and 20. Sorry for this – I'll update the CI config.

@jodeleeuw
Copy link
Member

@bjoluc can you peek at this when you get the chance? downgrading jspsych-config seems like it made a difference. guessing we should try to resolve this another way?

@jodeleeuw
Copy link
Member

@bjoluc great timing 😁 thanks!!

@bjoluc
Copy link
Member

bjoluc commented Dec 18, 2023

All set on the main branch – @marianylund can you merge the jspsych-contrib/main branch into your fork's main branch and revert commit 614c0ea? The CI in this PR should still be passing then with the new package versions.

@marianylund
Copy link
Contributor Author

@bjoluc Thank you, I have updated the fork and reverted the commit. Is there anything else I should do?

@jodeleeuw
Copy link
Member

Nope, looks good to go! Thank you again!

@jodeleeuw jodeleeuw merged commit d7f4685 into jspsych:main Dec 20, 2023
2 checks passed
@github-actions github-actions bot mentioned this pull request Dec 20, 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.

4 participants