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

Update images of guide/README.md #4272

Conversation

Ubayed-Bin-Sufian
Copy link
Contributor

@Ubayed-Bin-Sufian Ubayed-Bin-Sufian commented Jan 14, 2025

Summary

This Pull Request replaces the outdated images with the updated ones. It is a follow-up of #4243.

Changes introduced

  • Update to latest images

Checklist

  • The changes adhere to the project's contribution guidelines.
  • Code changes have been implemented and tested.

Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

@walterbender
Copy link
Member

I am not sure it is worth replacing the SVG with PNG embedded in SVG. The differences are really cosmetic in almost every case.

There are issues with the pitchslider images. The sliders are set to the same position even though the pitches are different. It doesn't make sense.

@Ubayed-Bin-Sufian
Copy link
Contributor Author

I am not sure it is worth replacing the SVG with PNG embedded in SVG. The differences are really cosmetic in almost every case.

There are issues with the pitchslider images. The sliders are set to the same position even though the pitches are different. It doesn't make sense.

I agree, the changes are trivial. I would change the core svg instead of embedding png in svg. Would you suggest a rebase for all the above images or a separate PR?

@walterbender
Copy link
Member

Your changes to the text of the README are good. We need to fix the pitchslider images (SVG version). I think we can drop the other changes. Might be easiest in a new PR.

@Ubayed-Bin-Sufian
Copy link
Contributor Author

I am not sure it is worth replacing the SVG with PNG embedded in SVG. The differences are really cosmetic in almost every case.

There are issues with the pitchslider images. The sliders are set to the same position even though the pitches are different. It doesn't make sense.

I have reviewed the guide, and it appears that the sliders are intended to be in the same position. I tested this behavior in both the master branch and the deployed version, and it seems to align with the original design intent.

  • The Pitch Slider widget creates one column per Sine block, meaning each frequency gets its own vertical slider.
  • Since (for eg: both 220 Hz and 440 Hz) are initial values for their respective sliders, they should appear aligned horizontally, starting at the middle or a neutral position of their individual columns.

@walterbender
Copy link
Member

OK.

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.

2 participants