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

Fix morty issue #1

Merged
merged 6 commits into from
Sep 27, 2021
Merged

Fix morty issue #1

merged 6 commits into from
Sep 27, 2021

Conversation

vaiavgm
Copy link
Contributor

@vaiavgm vaiavgm commented Sep 25, 2021

The Morty error will only be logged, if the user would expect (based on the 5s timer) that a previous track would be loaded.

@jurelik
Copy link
Owner

jurelik commented Sep 25, 2021

Thanks for the contribution! I think a better way to solve this issue is by simply changing the "Can't go back in time..." line from:

if (this.queuePosition <= 0) log.error("Can't go further back in time Morty.");

to

if (this.audio.currentTime < 5 && this.queuePosition <= 0) log.error("Can't go further back in time Morty.");

Might as well change <= 0 to < 1 while you're at it to keep things tidy :)

If you're happy doing that I'll gladly merge it into dev!

@vaiavgm
Copy link
Contributor Author

vaiavgm commented Sep 26, 2021

I disagree with your suggestion to perform the check in one line, because you'd be checking the same condition ( currentTime < 5 ) twice.
Especially, since "5" is a magic number (that should probably be a constant like const jumpToPrevTrackThreshold = 5;), it is more error prone when spread over multiple conditions that check the same thing.

We're only testing the pull-request system mainly; you can obviously patch it yourself in a much shorter time than us messing around with comments and such :)

If you disagree with my changes, let's just close the PR ^_^

@vaiavgm vaiavgm changed the base branch from main to dev September 26, 2021 08:24
@jurelik jurelik mentioned this pull request Sep 27, 2021
32 tasks
@jurelik jurelik merged commit ded88aa into jurelik:dev Sep 27, 2021
@jurelik
Copy link
Owner

jurelik commented Sep 27, 2021

On a second look, you're absolutely right. I merged this and I'll add a global threshold var for the time it takes before a backButton press changes functionality (currently hard-coded at 5 seconds).

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