jeroenheijmans / advent-of-code-charts

Inject charts in your private leaderboard page for Advent of Code
MIT License
118 stars 21 forks source link

Points leaderboard based on percentage #60

Closed svatal closed 1 year ago

svatal commented 2 years ago
svatal commented 2 years ago

I've also added JSDoc comments describing the input data as it greatly helped with the first contribution (no data: any)

jeroenheijmans commented 2 years ago

Interesting suggestion! I don't have immediate time to review it, but the idea seems solid to me. Thanks for contributing!

EDIT: This might well mean that I pick it up in November next year. Most years I prep a fresh release before a new edition, just to ensure I don't break things in the month that we're up and running.

Happy puzzling!

jeroenheijmans commented 2 years ago

By the way, you might be interested in this other plugin by @amochtar, which I believe gives you more or less the same graph (just more discrete with "leaderboard position" instead of cumulative points, but I think that actually looks quite nice). I've been using that plugin next to my own and they complement each other nicely IMHO.

svatal commented 2 years ago

Yes, I'm aware of that plugin and I'm using it too. It has been first place where I would have wanted to do this 'small' modification. But to display the distances you need a graph, not a table. Thus, your plugin has seemed like a better starting point.

svatal commented 2 years ago

I've tried to build on the change in PR and display the potential - what are maximum points reachable for each player if he finishes the remaining stars just now. I'm not really satisfied with the result, both the many lines and the second legend line for almost every player (only 3 players have finished all stars and thus does not need to display the potential) looks really messy. obrazek On the other side, it has a great value - you can easily see (after you make sense to the graph :)) that the third player will be probably knocked down by fourth one, once he finishes all remaining stars, but he is safe from the fifth one.

Any thoughts on this?

jeroenheijmans commented 2 years ago

Well, it looks very interesting at the least! But I agree it does seem a bit cluttered.

For the current moment I have a slightly different problem: I'm pretty burnt out on December and the projects around AoC for 2021, and I still really want to finish the final few days of puzzles spoiler-free myself in the upcoming time. So I'll likely let this and other PRs and issues sit until about November 2022, just before the next edition. I appreciate all the suggestions and input and will do my utmost best to look at all of them for the next AoC edition, promised!

Hope that sounds reasonable :)

svatal commented 2 years ago

Well, I do not mind longer pause, but since the "potential" feature is far from finished, I'd like to put some more work into it - and restarting it in november seems like there will be 'release' already knocking on the door. Could it be 'summer'? :)

Good luck and clear mind with the puzzles!

jeroenheijmans commented 1 year ago

I apologize @svatal: my intention truly was to look at this in November already, but life happened this fall, so no big updates to the repo or extensions this year I'm afraid. I do appreciate the submission though and hope to get to it after this AoC'22 at some point. Hope you understand!

svatal commented 1 year ago

Hello @jeroenheijmans, it is fine - I hope I'll be able to run my local build again. Life is much more important than any coding event and I wish you luck in sorting things out.

svatal commented 1 year ago

I've added the 'potential' as another view to cycle. So it is (header click to change) - cumulative points (the original) -> percentage points -> percentage points with potential -> back to cumulative points. This should solve the issue with too much info in one graph - everyone can pick the one that suits him/her.

So no open issues with this PR from my side .. :)

jeroenheijmans commented 1 year ago

I've just found the time to check this out, and it's absolutely lovely!

I will be merging this locally into my #79 (2023 updates) branch, because I'll need to juggle some conflicts with other PR's that I've also accepted. But I'll make sure your line of commits gets in there, and it should land in the next edition.

I am planning to tweak a few things on top of your work, including:

Thanks again so much! 🙏🏻 And expect to see your features in the released addon in the future.

svatal commented 1 year ago

Thank you very much! But I'd like you to reconsider removing the JSDocs. As you can see, it has been my first commit - I wouldn't be able to even start without knowing what the data consist of. They add an enormous value (well, maybe I'm a little biased since I'm used to writing typescript everyday ;)). There does not need to be much declarations - basicaly just the input JSON. The IStar and IMember (and some in-function typings) could probably be replaced with some restructuralization of the code like not mutating IMemberJson but creating a new object (lines 143-180) or constructs like Math.max(..array) instead of Math.max.apply(Math, array. My intent in that phase has been exploring the data, not refactorings that would make it more readable for TS language service but may break something I don't yet understand.

I can try out the proposed changes, but it would complicate the merging ...

jeroenheijmans commented 1 year ago

Ha, no @svatal: thank you very much! 😄

First things first: take care not to add commits to this PR anymore 😅, I've already merged your work into the branch for 2023 updates 🎉 and manually made further changes, and solved tons of conflicts with PRs from others. If you'd add more commits to this PR we'll likely run into nasty merge conflicts. (I welcome further help!! But just want to avoid those git conflicts for both of us.)

Second, I understand your plea for JSDocs. However, I want to keep that discussion separate (and merge this PR separately from JSDocs, if any). I've opened #82 and will give it some more thought.

jeroenheijmans commented 1 year ago

As notifications may've told you, this PR is now merged into the mainline! Further PR's should probably branch off that, and the JSDocs thing has its own open issue for now. Thanks again for your contribution!