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

Show song info in title #393

Merged
merged 4 commits into from
Jun 10, 2019
Merged

Show song info in title #393

merged 4 commits into from
Jun 10, 2019

Conversation

mmatous
Copy link
Contributor

@mmatous mmatous commented May 4, 2019

Hi, I wrote this basically one-liner feature and did some light refactoring and fixing along the way, so I would not have to duplicate code.
You may not want the last commit as I did not see you using let, const or template literals anywhere in JS, but I don't know whether you are actively avoiding them or just not used to using them.

@Rello
Copy link
Owner

Rello commented May 5, 2019

Hello,
thank you! I will have a look.

the reason against LET is the compatibility.
older iOS devices for example don´t support it. as my primary usecase for AP are the kids` older iPads, this part can not be implemented.

@mmatous
Copy link
Contributor Author

mmatous commented May 6, 2019

2nd version
Added missing semicolon in my code.
Swapped lets for vars.
Did I understand correctly that you use snake case for sort or private helper methods and camel case for what would be public interface? I was a bit confused about seeing both in one file and did not know what to use for my function.

@Rello
Copy link
Owner

Rello commented May 6, 2019

snake case for sort or private helper methods and camel case
Hey,
thank you for coming back. as you might notice, I am not a pro JS dev.
If you have any suggestion how to do it correct and consistent across the whole app, I will be happy to accept the suggestions and do a proper cleanup!

are you referring to something like this?
https://en.wikipedia.org/wiki/Naming_convention_(programming)#Java

@mmatous
Copy link
Contributor Author

mmatous commented May 6, 2019

are you referring to something like this?
https://en.wikipedia.org/wiki/Naming_convention_(programming)#Java

Yes, pretty much. Just like in any other language pick one thing and stick with it. Does not mattern what you pick in the end, as long as you are consistent. If you use a style that is commonly known and accepted even better. Even better than that, use eslint and push eslint config file in your repo so anyone can see in their IDEs what you want. You might have to tweak the file a bit since you use jQuery.
In addition eslint might reveal other problems in your code.

@Rello Rello merged commit 7affd10 into Rello:master Jun 10, 2019
Rello added a commit that referenced this pull request Jun 10, 2019
Rello added a commit that referenced this pull request Jun 10, 2019
Rello added a commit that referenced this pull request Jun 11, 2019
Rello added a commit that referenced this pull request Jun 11, 2019
@Rello Rello added this to the 2.7.1 milestone Jun 13, 2019
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