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

Modified Visual Search Circle Plugin #1954

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

keshavpabbi
Copy link

I have replaced the visual-search-circle plugin with a modified version of the plugin called visual-search. This plugin allows experimenters to run a visual search experiment with images placed on a grid if they choose not to use the circle format. I have also added a jitter parameter that can be used by experimenters.

(jitter_ratio and usegrid are the 2 parameters I have added to the visual-search plugin)

@jodeleeuw
Copy link
Member

Thanks @keshavpabbi !

Would you be willing to modify the corresponding pages in /docs as part of the PR?

@jodeleeuw jodeleeuw added this to the 7.1 milestone Jun 30, 2021
@keshavpabbi
Copy link
Author

Thanks @keshavpabbi !

Would you be willing to modify the corresponding pages in /docs as part of the PR?

@keshavpabbi keshavpabbi closed this Jul 2, 2021
@keshavpabbi
Copy link
Author

Thanks @keshavpabbi !
Would you be willing to modify the corresponding pages in /docs as part of the PR?

Sure! Is there anything specific I should modify in the docs folder? Also I closed this by accident. Do I have to submit another pull request?

@becky-gilbert becky-gilbert reopened this Jul 2, 2021
@becky-gilbert
Copy link
Collaborator

Hi @keshavpabbi, I just re-opened this 😃

I think the documentation pages that need editing are:

  • docs/plugins/list-of-plugins.md

    • change plugin name in the list
  • docs/plugins/jspsych-visual-search-circle.md

    • rename the file
    • add your new parameters to the parameters table (along with their defaults, descriptions etc.), and change the default value for the circle diameter
    • update the Examples section

I had a quick look at your changes to the plugin file and didn't notice any other changes to how the plugin works. But if you made any other changes then please mention those on the docs page as well.

Another thing to note is that we're currently making changes to the Examples sections in the documentation pages anyway, which you can read about here: #1923 and here: #164. So if you want to incorporate these changes to the visual search documentation page too, then that would be great, but no problem if not!

Thanks!!

keshavpabbi and others added 9 commits July 2, 2021 20:49
changed the name of visual-search-circle to visual-search and changed the description to show new code added to the plugin
renamed visual-search-circle to visual-search, changed description for the plugin
@becky-gilbert
Copy link
Collaborator

Hi @keshavpabbi, thanks very much for editing the documentation pages! I'll just test the way your updated version of this plugin works. If I have any questions or issues, I'll let you know, otherwise this looks ready to merge 👍

@becky-gilbert
Copy link
Collaborator

And don't worry about the failing Jest test. If you want to, then feel free to fix whatever test is failing and include any plugin or test file modifications in this PR. But if you don't want to or aren't sure how then I can do it.

@becky-gilbert
Copy link
Collaborator

Ah, I just checked the Jest test errors and it looks like an easy fix: the old plugin name is used in the file "tests/plugins/plugin-visual-search-circle.test.js". So I'm guessing the tests will pass when this is changed. Again, feel free to change it and include it in this PR, otherwise I can do it 😃

@keshavpabbi
Copy link
Author

Ah, I just checked the Jest test errors and it looks like an easy fix: the old plugin name is used in the file "tests/plugins/plugin-visual-search-circle.test.js". So I'm guessing the tests will pass when this is changed. Again, feel free to change it and include it in this PR, otherwise I can do it 😃

So just to confirm, am I supposed to change the name of the js file?

@becky-gilbert
Copy link
Collaborator

Sorry for not being clear. The name of the file does need to be changed, but the error is actually caused by references to the old plugin name inside of that file. Let me know if you have any other questions or if you'd like me to help out with these final changes. Thank you!

…l-search.test.js

replaced "visual-search-circle" with "visual-search"
@keshavpabbi
Copy link
Author

Sorry for not being clear. The name of the file does need to be changed, but the error is actually caused by references to the old plugin name inside of that file. Let me know if you have any other questions or if you'd like me to help out with these final changes. Thank you!

Alright so I changed the name of the file and wherever it said "visual-search-circle" in the code, I replaced it with "visual-search". Is there anything else I should fix?

@becky-gilbert
Copy link
Collaborator

Yep that's perfect, thanks!

Note that this is marked for the 7.1 release. Right now we're working some other changes for the next release (7.0), and then we'll switch to 7.1, which will be focused on adding and improving plugins. So this pull request will be merged sometime after we've finished 7.0, which I'm guessing will be in about a month.

@becky-gilbert becky-gilbert self-assigned this Jul 7, 2021
@keshavpabbi
Copy link
Author

Yep that's perfect, thanks!

Note that this is marked for the 7.1 release. Right now we're working some other changes for the next release (7.0), and then we'll switch to 7.1, which will be focused on adding and improving plugins. So this pull request will be merged sometime after we've finished 7.0, which I'm guessing will be in about a month.

Sounds good Becky!

@jodeleeuw
Copy link
Member

Hi @keshavpabbi,

We created a new community contributions repository that would be a great immediate home for this plugin. We'll definitely keep this on the agenda for merging in the long term, but we have some other priorities so it'll be a few months at least.

@jodeleeuw jodeleeuw removed this from the 7.1 milestone Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants