hvianna / audioMotion-analyzer

High-resolution real-time graphic audio spectrum analyzer JavaScript module with no dependencies.
https://audioMotion.dev
GNU Affero General Public License v3.0
643 stars 63 forks source link

Improve naming of toggleAnalyzer parameter #55

Closed Staijn1 closed 1 year ago

Staijn1 commented 1 year ago

Hey!

I found myself wondering if the parameter of toggleAnalyzer enabled, or disabled the analyzer. The only thing this PR does is change it's name to be more descriptive, and add some comments.

I don't think the README.md needs to be updated.

hvianna commented 1 year ago

Hey @Staijn1 !

Thanks for the PR. I agree the parameter name should be more clear (I suck at choosing variable names 😅), but I think mustEnable can be misleading as well, because when it's set to false it will actually stop the analyzer.

For similar native methods, force seems to be a standard parameter name in the JavaScript docs:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/togglePopover https://developer.mozilla.org/en-US/docs/Web/API/Element/toggleAttribute https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle

And jQuery uses state for this parameter name in a similar method, which also seems like a good choice.

https://api.jquery.com/toggleClass/#toggleClass-className-state

What do you think?

Staijn1 commented 1 year ago

I will admit, I was not completely sold on mustEnable either..

I asked my, (maybe our) best friend ChatGPT for any other suggestions and he came back with the following:

However, judging by the MDN documentation I think we should go with force to be consistent with native Javascript functions. Although activate in my opinion is not bad either.

hvianna commented 1 year ago

Sticking with force seems good to me!

Staijn1 commented 1 year ago

Suggested changes are pushed! :)