Conversation
JoaoBGusmao
left a comment
JoaoBGusmao
left a comment
There was a problem hiding this comment.
I'm not the project owner, but I did a small code review to help keeping the code beautiful as it is today
| const AudioManager = {}; | ||
|
|
||
| AudioManager.playBGM = async function(name) { | ||
| var musicVolume = 1; |
There was a problem hiding this comment.
It's not recommended to use var. Try using const instead
There was a problem hiding this comment.
This is correct. Take a look at this style guide: https://github.com/airbnb/javascript.
There was a problem hiding this comment.
As an addendum, I don't recommend storing these variables at the module level. I prefer assigning these variables to the AudoManager object.
|
|
||
| function PLAY_AUDIO(audio, vol) { | ||
| const concurrentAudio = audio.cloneNode(); | ||
| concurrentAudio.volume = volume; |
There was a problem hiding this comment.
Why change volume to vol? It's a good practice to be explicit in what you want to do, and abbreviating names could bad for readability
There was a problem hiding this comment.
👍 Also, the default parameter is there for a good reason.
| @@ -0,0 +1,5 @@ | |||
| { | |||
There was a problem hiding this comment.
Why is this VS config file here?
There was a problem hiding this comment.
To be honest I don't know. He is on my .gitignore. Gotta remove for the next commit
| * `npm install` | ||
| * `npm run local` to start the application with inspectable client code. | ||
| * `npm run dev` to start the application with minified IIFE client code. | ||
|
|
There was a problem hiding this comment.
This is my comment on the RZ thread. I don't think it should be part of the README.
| } | ||
| }; | ||
|
|
||
| AudioManager.setMusicVolume = async function (vol) { |
There was a problem hiding this comment.
The async is not necessary because you're not using await in the body of the function. Also, don't do type checking here; it's an antipattern and the caller is expected to pass a number.
There was a problem hiding this comment.
Got it, I was trying to make some sort of validation
|
|
||
| const hatVslot = !hat? '' : hat.info.vslot.nValue; | ||
| const hatParts = !hat? [] : getParts(hat).filter(isDrawable); | ||
| const hatVslot = !hat ? '' : hat.info.vslot.nValue; |
There was a problem hiding this comment.
These ternaries are the only places where VS autoformat is acceptable. See the other comment about style.
| const UICommon = {}; | ||
|
|
||
| UICommon.initialize = async function() { | ||
| UICommon.initialize = async function () { |
There was a problem hiding this comment.
Please no whitespace between function and the parentheses.
There was a problem hiding this comment.
@johncintron according to airbnb styleguide, the space is correct:
space-before-function-paren
|
|
||
| if (this.activeButton === this.loginButton) { | ||
| UICommon.playMouseHoverAudio(); | ||
| UICommon.playMouseHoverAudio(1); |
There was a problem hiding this comment.
UICommon.playMouseHoverAudio doesn't take any arguments. Not sure why you're passing 1 to it.
There was a problem hiding this comment.
Probably forgot to remove, I was using during the test phase
|
Sorry for the mistakes, it's my first contribution to a public git. |
|
@igorpwnd keep working on this pull request, and send it again to @johncintron. Once you're done with corrections quoted above, he can maybe accept your request! |
3 participants
Most of my changes were at AudioManager.
I made 2 variables to control the sound volume, everytime that PLAY_AUDIO() is called, the sound volume is controled by one of those variables.
The rest of changes are identation and stuff from VSCODE