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

Add HowlGlobal Volume (With example) #19

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

Add HowlGlobal Volume (With example) #19

wants to merge 2 commits into from

Conversation

grierson
Copy link

No description provided.

Copy link
Owner

@StefH StefH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Volume(...) method should also be added to the https://github.com/StefH/Howler.Blazor/blob/master/src/Howler.Blazor/Components/IHowl.cs to enable setting the volume for a specific sound.

@grierson grierson requested a review from StefH June 15, 2021 13:08
Copy link
Owner

@StefH StefH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some more requested changes...

@@ -47,12 +47,25 @@
<td><button class="btn btn-primary oi oi-arrow-bottom" @onclick="SpeedDown"></button></td>
<td>Speed Down</td>
</tr>
<tr>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also update all 3 example projects?
image

@@ -245,4 +261,24 @@
await Howl.Pause(id);
}
}
private async Task VolumeUp()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a new line before this method

private async Task VolumeUp()
{
Volume += 0.1;
// await HowlGlobal.Volume(Volume);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe keep the await HowlGlobal.Volume(Volume); and put the foreach loop in comment?

@@ -105,6 +105,11 @@ public ValueTask<bool> IsPlaying(int soundId)
return _runtime.InvokeAsync<bool>("howl.getIsPlaying", soundId);
}

public ValueTask<double> Volume(double volume, int soundId)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

soundId should be first argument

@@ -48,6 +48,8 @@ public interface IHowl : IHowlEvents
ValueTask<TimeSpan> GetTotalTime(int soundId);

ValueTask<bool> IsPlaying(int soundId);
ValueTask<double> Volume(double volume, int soundId);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a newline before

@@ -48,6 +48,8 @@ public interface IHowl : IHowlEvents
ValueTask<TimeSpan> GetTotalTime(int soundId);

ValueTask<bool> IsPlaying(int soundId);
ValueTask<double> Volume(double volume, int soundId);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove newline after

@@ -17,5 +17,6 @@ public interface IHowlGlobal
ValueTask<string[]> GetCodecs();

ValueTask<bool> IsCodecSupported(string? extension);
ValueTask Volume(double volume);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add new line before

return howl.volume(volume, id);
}

return 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't return 0 here, that is not needed

@StefH
Copy link
Owner

StefH commented Jun 18, 2021

@grierson
Can you please take a look at my comments?

@StefH
Copy link
Owner

StefH commented Jun 24, 2021

Hello @grierson ; did you have time to take a look at my code review comments?

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