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

Added streak status #2

Merged
merged 1 commit into from
Nov 5, 2021
Merged

Added streak status #2

merged 1 commit into from
Nov 5, 2021

Conversation

m0nkiii
Copy link
Contributor

@m0nkiii m0nkiii commented Nov 1, 2021

  • Artist streak
  • Track streak
  • Album streak

First try and I think I manage to do some good.

@m0nkiii
Copy link
Contributor Author

m0nkiii commented Nov 1, 2021

Hmm, I'm not sure about that giant package-lock.json file :)
I noticed it too late :P

Maybe I should just redo the pull request tomorrow :D

@felhag
Copy link
Owner

felhag commented Nov 2, 2021

Looks really good, definitely a nice addition to the site. Got some minor optimizations though:

  • The other lists are mostly formatted like this: item (x times), for example Queen (20 times). Think it would be consistent to apply same style here too.
  • It doesn't have an onclick link, which isn't mandatory but is useful I think. For example to the first day of the streak of the corresponding item (I guess streaks don't often last longer than a day right?).
  • An explanation to the new lists also would be nice :)
  • The album list can be moved next to the previous list.
  • Regarding the code; I think the different stacks can be merged to a single class with a filter function
  • And if you're gonna modify your pr anyways maybe discard changes for the package.lock indeed ;-)

@felhag felhag merged commit 46e6c37 into felhag:master Nov 5, 2021
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