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

Resolved conflicts in PR #544 (volume blocks) #2289

Closed
wants to merge 22 commits into from

Conversation

@bromagosa
Copy link
Collaborator

commented Dec 12, 2018

No description provided.

@jmoenig

This comment has been minimized.

Copy link
Owner

commented Dec 13, 2018

Nice! This seems like a bunch of big changes, we'll want to thoroughly test them, right?
Did you add a mute button? Are you sure we want our own mute button rather than the one that's on your computer?
Thank you, I'm looking forward to playing with this.
Another question: Is the volume global, per sprite or even per script?

@bromagosa

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 14, 2018

@brianharvey

This comment has been minimized.

Copy link
Collaborator

commented Dec 14, 2018

@jmoenig

This comment has been minimized.

Copy link
Owner

commented Dec 14, 2018

Haha, they do :-)

@jguille2

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

Hi,
Some comments about this volume/mute feature:

  • I see "volume" is here an sprite property. Then "play sounds" and "play notes" are modified to use it. But now, we have some libraries ("Text to speech" and "Audio comp") that play sounds. So we have to decide if this "volume" is only for "Sounds" (like "size" is for the costumes) or it is really an sprite property, and then, we will have to update these libraries to add the volume feature.

  • We have to check that code, because it stores volume property into the Sound object. This is not right, because the volume is only a sprite property. This problem causes, for example, that "Import sounds" feature is broken with this PR. When you want to import a Sound (for example, from the sounds library), the import action is broken because "volume" property is not defined. The problem is in the "Sound.prototype.copy" function.

  • About mute, different comments:

    • First, the same comment about "other sounds" in Snap. If we want a really "mute" feature, we have to consider all Snap! sounds (text to speech.... and even the play action into the Sounds tab.
    • I'm not sure about this mute feature... but maybe it is useful thinking in the embed presentation (for our next social web). If we think in pages with different Snap! projects, maybe a "mute" feature can be useful. Then I like a button and also a url flag for the initial value (mute or not).
    • A last comment... maybe the "mute" icon is not clear.

Joan

@bromagosa

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2019

Closing as this feature has been added to dev already 👍

@bromagosa bromagosa closed this Apr 8, 2019
@bromagosa bromagosa deleted the bromagosa:fixed-PR-544 branch Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.