-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Update p5.sound toggle behaviour #3522
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
base: develop
Are you sure you want to change the base?
Conversation
'https://cdnjs.cloudflare.com/ajax/libs/p5.js/$VERSION/addons/p5.sound.min.js'; | ||
export const p5SoundURLOld = p5SoundURLOldTemplate.replace( | ||
'$VERSION', | ||
'1.11.3' |
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.
@davepagurek Would it be possible to use latest within 1.x? Since we are still bugfixing for a while.
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.
Updated this to use currentP5Version
. I also updated the default web editor version to 1.11.8, since it was still 1.11.7 elsewhere. @raclim ideally we use the same string and splice it into createDefaultFiles.js
so we have less places to update when there's a new version, as currently the two were out of date. Looks like that file is run on the server? Is there a spot for anything currently that gets imported in both client and server that I could move that constant to?
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 believe that it exists yet. I'm sorry I think it might have to be created!
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.
No problem! I'm happy to create that. Would something like a top-level common
folder be ok?
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, that sounds good to me! Thank you so much!
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 think I've added that! i added a thing to the dockerfile about the common
folder, which seems to work running the project via docker, but I'm less confident that that's correct or necessary, so let me know if that seems ok to you!
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 I think its correct, thanks so much for catching that I completely forgot about it! The folder setup/Dockerfile looks good to me! :)
This overall looks great to me!!! This is small question—I noticed that if I have a new sketch, switch to 2.x, and then back to 1.x, it seems like p5.sound is still turned off. I was wondering if this is intentional, or something that we might want to address here as well? |
Currently intentional, but we can talk about what we'd need to do if we want that! Our previous "remembering" logic was intended to try to capture the specific p5.sound URL that was being used, but that prevented you from switching to 2.0 and then turning on the 2.0 version of p5.sound. We could possibly do something where we have separate memory for 1.x and 2.x, and only try to restore when you go back and forth between them? That could possibly be a memory of a specific version like before (downside being, it's harder to switch between 1.x versions and update p5.sound accordingly), or just on/off and we pick the corresponding version for you (downside being, it may not correspond to the version you used to be using, especially if you revert to a different 1.x than you started with.) |
Fixes #3513
Changes:
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123