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

Shuffle visualization preset feature #1555 #1612

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CarlosBor
Copy link
Contributor

A couple matters:

Regarding the third point of the guidelines, "If you're fixing a bug, or adding a new functionality", this is a new functionality, but when checking the tests for the visualizerOverlay component there are only snapshots. I've updated them, but should I add tests on it and the randomizing function I'm using or is it understood to be simple enough to be ok as it is?

Also, as it is right now, the name of the preset is stored in the "settings" slice of the redux store, I've thus added the shuffle toggle to the same slice in a similar way, is that okay or should I change it to use a new "visualizer" slice?

Besides that, the button to toggle shuffle appears like so:
image

And the shuffle happens if enabled when a track ends.

Besides that, awaiting for revision, thanks for your patience.

@nukeop
Copy link
Owner

nukeop commented Jun 1, 2024

Hi thanks for contributing this. I will do a review and comment in the files individually, but it would be great to have tests that reproduce the behavior of the user using this function, so something that mounts the visualizer, selects the shuffle mode, waits for the end of the current song to see if it's changed, etc. Basically everything you added in this PR.

@nukeop
Copy link
Owner

nukeop commented Jun 1, 2024

I looked at your code and I have no additional remarks. It's all great.

@CarlosBor
Copy link
Contributor Author

Went with the pull request update rather than writing into Discord this time, should be tidier this way.

Long story short, I did manage to mock the library but I reckon that was the wrong way to go about things, I just can't get the handleFinishedPlaying() function from the SoundContainer component to fire. I'm guessing it ultimately triggers as a response to an event fired somewhere but I'm lost regarding how to simulate the behavior and frankly I have the suspicion that there is something obvious that I'm missing.

I'd be thankful If you could take a crack at it in case it's not too much work. Otherwise I could really do with some pointers.

Thanks for your patience.

@nukeop
Copy link
Owner

nukeop commented Jun 15, 2024

Hey, I'm working on fixing the Windows build first. I'll take a look as soon as I can. I'm sure we'll figure it out together somehow.

@CarlosBor
Copy link
Contributor Author

Sure thing. I use Windows so if I can offer any assistance I'm up for it, don't know much about the matter though.

In the meantime I'll take a small gander at #1539, will comment in that thread if I start taking care of it proper.

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.

None yet

2 participants