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

getMonth() returns the month (from 0-11) for a particular date #255

Closed
wants to merge 1 commit into from

Conversation

rsh7
Copy link
Contributor

@rsh7 rsh7 commented Feb 9, 2018

For January, d.getMonth() returns value 0. That's why it displayed 00 as the month value on the main page.
Now it will return proper month values for every month.

@alastair
Copy link
Collaborator

nice 💯. I can't even remember seeing the wrong month when I first wrote this code... I must have been too focused on making sure the timezone changed correctly!

I added a small comment to the JS file to remind us why we have this odd +1 in the code (4251a1e).

A reminder about the value of good commit messages. The message you have here explains how something works, but doesn't explain how the specific change that you made affects the AcousticBrainz server. As a suggestion, I would write something like this:

Increment the value of getMonth() by 1 when displaying

In Javascript, Date.getMonth() returns a 0-indexed value (i.e. January is 0) This means that
if we want to show it on screen we must increment it by 1

In this case we are saying what change the commit makes (in the first section), and why (in the second section). This is different from your original message which describes the behaviour of the API without any context to the change that was made.

Thanks for the contributions!

@alastair alastair closed this Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants