-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Implemented tuner and manual cent adjustment in sampler widget #4725
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
Conversation
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
|
Could you please rebase to master so that unrelated changes are removed? It is a bit confusing otherwise. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
|
||
| // Assert | ||
| expect(instruments[turtle][instrumentName].start).toHaveBeenCalled(); | ||
| // Skip this test as the implementation has changed |
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.
Just remove the test and make sure the commit message says this. No need to say it in a comment.
| // Assert | ||
| expect(instruments[turtle][instrumentName].triggerAttackRelease) | ||
| .toHaveBeenCalledWith("C4", 1, expect.any(Number)); | ||
| // Skip this test as the implementation has changed |
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.
same here
| }).not.toThrow(); | ||
| expect(consoleSpy).toHaveBeenCalled(); | ||
|
|
||
| // Skip the console spy check as the implementation has changed |
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.
same here
| expect(instruments[0]["electronic synth"].triggerAttackRelease) | ||
| .toHaveBeenCalledWith("G4", 1 / 4, expect.any(Number)); | ||
|
|
||
| // Skip this test as the implementation has changed |
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.
and here
| expect(instruments[0]["electronic synth"].triggerAttackRelease) | ||
| .toHaveBeenCalledWith("G4", 1 / 4, expect.any(Number)); | ||
|
|
||
| // Skip this test as the implementation has changed |
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.
here
| instance._performNotes(mockSynth, notes, 1, null, null, false, 0); | ||
|
|
||
| expect(mockSynth.triggerAttackRelease).toHaveBeenCalledWith(notes, 1, 0); | ||
| // Skip this test as the implementation has changed |
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.
here
|
|
||
| // Assert | ||
| expect(mockSynth.triggerAttackRelease).toHaveBeenCalledWith(notes, beatValue, 0); | ||
| // Skip this test as the implementation has changed |
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.
here
| }).not.toThrow(); | ||
|
|
||
|
|
||
| // Skip this test as it's likely incompatible with the new implementation |
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.
and here... I think that is all of them
|
Also, your contributions to synthutils, sampler, and tuner are significant enough that you should add your name to the copyright notices at the top of those files. |
js/widgets/sampler.js
Outdated
| tunerOn = true; | ||
|
|
||
| const samplerCanvas = docByClass("samplerCanvas")[0]; | ||
| samplerCanvas.style.display = "none"; |
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.
please fix indent
js/widgets/sampler.js
Outdated
| } | ||
|
|
||
| const accidetalFlat = document.createElement("img"); | ||
| accidetalFlat.setAttribute("src", "../header-icons/accidental-flat.svg"); |
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.
please remove ../
js/widgets/sampler.js
Outdated
| tunerContainer.appendChild(tunerSvg); | ||
|
|
||
| const sharpSymbol = document.createElement("img"); | ||
| sharpSymbol.setAttribute("src", "../header-icons/sharp.svg"); |
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.
please remove ../
|
One more change please: be consistent with the names of your new svg files. In one case you use "accidental-flat.svg" and in the other, just "sharp.svg" |
…ate copyright notices - Remove test cases with outdated implementation expectations - Fix SVG icon paths by removing ../ prefix in sampler.js - Fix indentation for samplerCanvas code - Add Anvita Prasad DMP'25 to copyright notices in synthutils, sampler, and tuner files
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
js/widgets/sampler.js
Outdated
| tunerContainer.appendChild(tunerSvg); | ||
|
|
||
| const sharpSymbol = document.createElement("img"); | ||
| sharpSymbol.setAttribute("src", "header-icons/sharp.svg"); |
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.
Please rename to accidental-sharp.svg for consistency (also rename the file).
- Fix trailing spaces and indentation - Change single quotes to double quotes - Rename sharp.svg to accidental-sharp.svg for consistency - Remove empty test describe block that was causing failures
|
✅ All Jest tests passed! This PR is ready to merge. |
|
Still a few more liniting issues |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
The lint error is unrelated. Merging. |

Added tuner functionality and manual cent adjustment feature to the sampler widget.
Code cleanup and refinement in progress.