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

[WIP] p5.sound #581

Merged
merged 5 commits into from
Oct 10, 2024
Merged

[WIP] p5.sound #581

merged 5 commits into from
Oct 10, 2024

Conversation

davepagurek
Copy link
Collaborator

@davepagurek davepagurek commented Oct 2, 2024

About to get on a flight back to Toronto, but @limzykenneth this is what I was tinkering with while I was waiting to board if it's useful!

@davepagurek davepagurek marked this pull request as draft October 2, 2024 21:45
@limzykenneth
Copy link
Member

limzykenneth commented Oct 3, 2024

That's pretty quick @davepagurek, thanks! I think something like this would be the general idea yes.

I briefly mentioned in the email but we would probably want to be able to plug in custom data.json to be able to preview changes to documentation instead of having to push to the repo and pull before building the website. Some parts of the docs here can probably be split to enable that as a script perhaps.

@davepagurek
Copy link
Collaborator Author

Ok, so it looks like this successfully pulls in reference data from both repos now! The one main change that was needed in p5.sound was to use doc comments with two **s at the start in the blocks that declare @module p5.sound (mentioned in processing/p5.sound.js#37 (comment)); for now I've made a fork that does that manually just so I could test that everything from that point on still works. Before merging this PR, once the p5.sound repo is updated, I need to change the git URL and branch back. I also haven't committed all the new files that get generated since they don't yet have the p5. prefix on the class names, I also mentioned that in the PR review.

Next question is probably how we want to display the sound reference. If we want it as a separate page (which I assume we do since that's how it worked on the old site?) I'll probably need to do a fair amount more work because the reference isn't currently set up for that. I think the plan would be to set up a separate page (unsure yet if I can make it in the reference subdirectory without the router clashing, maybe /libraries/sound like on the old site will suffice?), filter down the reference items, and pass it in to the same layout as the current reference. I imagine it might need some UI updates since it would just have one module in the lower-left nav panel by default. That may need to wait until Thursday for me to work on that as I won't quite have enough time tomorrow or Wednesday.

In the mean time, the fastest way to keep testing was to just let p5.sound show up as another module in the reference, since the data is set up for it to be that currently:

image

I've also added some code to include a <script> tag to the cdn's lib/addons/p5.sound.js file for all examples in the p5.sound module. It doesn't all fully work yet since the new repo isn't published there yet. Are we planning on publishing it under the p5.sound name going forward to let its releases not be tied to the p5 releases?

cc @Qianqianye

@davepagurek
Copy link
Collaborator Author

Update: routing works! so we are hosting the sound reference at /reference/p5.sound.

Since it's all in one module, I've turned the subcategories (in this case, classes) into the jump links on the side.

image

The p5.sound classes don't have a p5. prefix in their own repo. Simply changing the docs to @class p5.Amplitude instead of just @class Amplitude makes yuidoc produce outputs for both p5.Amplitude and Amplitude, I think because we still export the non-prefixed version. So instead Im just adding the prefix in this repo when we combine the docs.

One other note, in order for sound examples to work, the sound script tag must appear after the p5 script tag. However, we currently inject the p5 script tag. Once injected, it immediately starts running the example, before p5.sound has a chance to load if we add it in a similar way. Instead, if an example needs sound, I just incldue the cdn link directly in the source code instead of doing the injection method -- less efficient, but it's only for sound examples.

@davepagurek davepagurek marked this pull request as ready for review October 10, 2024 21:34
@Qianqianye
Copy link
Collaborator

This is amazing, thanks @davepagurek!

@Qianqianye Qianqianye merged commit a61cf25 into main Oct 10, 2024
4 checks passed
@Qianqianye Qianqianye deleted the p5.sound branch October 10, 2024 23:04
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