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

Create countdown extension #90

Merged
merged 11 commits into from
Jan 11, 2024
Merged

Create countdown extension #90

merged 11 commits into from
Jan 11, 2024

Conversation

Shaobin-Jiang
Copy link
Contributor

I have frequently encounted cases where I need to add a countdown display to a trial. Therefore, I have created this countdown extension to enable this without making a mess of the code (previously there was a lot of extra work to be done in on_load and on_finish).

Copy link

changeset-bot bot commented Jan 6, 2024

🦋 Changeset detected

Latest commit: 8a7f30d

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

This PR includes changesets to release 1 package
Name Type
@jspsych-contrib/extension-countdown 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

@jodeleeuw
Copy link
Member

Awesome 🥳

Can you update the main README to add this to the list?

I think it would also be useful in the docs to be clear when the countdown occurs (before/during trial?)

@Shaobin-Jiang
Copy link
Contributor Author

I made the updates. But I do not see why the checks should fail when they passed before I made these changes. After all, I did not even make any changes to the code.

I took a look at the log and I saw that the error occurred in the video-several-keyboard-responses plugin, but that pr was merged weeks ago, so how could it still affect the current check?

@jodeleeuw
Copy link
Member

Not sure what happened with the checks. I'm guessing one of the tests in the video-several-keyboard-response plugin has some kind of randomization built in and failed due to an outlier case.

Copy link
Member

@jodeleeuw jodeleeuw left a comment

Choose a reason for hiding this comment

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

Just a couple quick things in the package file.

packages/extension-countdown/package.json Outdated Show resolved Hide resolved
packages/extension-countdown/package.json Outdated Show resolved Hide resolved
@bjoluc
Copy link
Member

bjoluc commented Jan 10, 2024

@jodeleeuw This already happened once in the video-several-keyboard-response PR's workflow runs and I spent half an hour trying to replicate the test failure on my local machine back then but wasn't able to get the test to fail even once 🌚 Any ideas what might be causing it? I certainly don't have any 🙈

@jodeleeuw
Copy link
Member

I think the problem is that the simulate method wasn't updated to reflect the possibility of multiple responses. The solution might be to remove the simulation capability for now, or see if @marianylund is willing to work on the fix.

@jodeleeuw jodeleeuw merged commit 3cb9958 into jspsych:main Jan 11, 2024
2 checks passed
@github-actions github-actions bot mentioned this pull request Jan 11, 2024
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.

3 participants