Skip to content

Conversation

@gracetian6
Copy link
Contributor

@gracetian6 gracetian6 commented Jul 31, 2020

image

image

@gracetian6 gracetian6 self-assigned this Jul 31, 2020
@gracetian6 gracetian6 marked this pull request as ready for review August 4, 2020 02:19
Copy link
Collaborator

@seunomonije seunomonije left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* renders heat map onto html
* @param {number[]} data array of all 0s/1s heat map data
* @param {number} dataLength size of data array
* @param {number[]} allBinaryData array of all 0s/1s heat map data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider renaming to just binaryData, allBinaryData insinuates that there's some other array with just a segment of the data, which there isn't so we can simplify this name


createHeatMap(likedMusicBinaryHist, TOTAL_LIKED);
GENRE_ANALYSIS.then((genreAnalysisInfo) => {
const LIKED_MUSIC_BINARY_HIST = makeBinaryArr(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think this is a true constant, it should just use lowerCamelCase

return JSON.parse(genreCount);
}

export const GENRE_ANALYSIS_PROMISE = fetchMusicGenre();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See earlier PR about not calling a function with side effects (such as making a web request) on import

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.

5 participants