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

Tick Climbs and Show Stats #72

Merged
merged 33 commits into from
Aug 29, 2024
Merged

Conversation

gardaholm
Copy link
Contributor

@gardaholm gardaholm commented Aug 12, 2024

This PR addresses #46, #54, #55 and #73
For testing I've put this branch online on fly.to: climbdex.why.dev

Menu Elements and Result View

Dropdown / Hamburger Menu is shown to clean up the Result view a bit. If user is logged in there is an option to logout, tick climb and show stats. If user user is not logged in there is only the beta information available.
Result List marks Sends in green and Attempts in yellow, to give a quick information about the status of the climb.

  • To support this view the Boardlib PR Aggregated Logbook  BoardLib#41 is needed to fetch and collect the logbook in the correct way. I've currently set my Branch in the requirements.txt for testing.
image

Simple Climbing and Attempt Stats

If the user is logged in, a separate menu item for Statistics is shown. This modal shows amount of attempts and sessions and when the last session was in a very basic form. I guess this place could be used for further improvements like suggested in Issue #28

image

Tick Modal

Ticking climbs can be done via the new modal, which should be way faster than compared to the aurora app. Ticking a Climb is done via the boardlib function save_ascent. Difficulty is prefilled but can be changed. Number of attempts can be set when the slider is set to Send instead of Flash.

  • Only Ascents are currently possible, but it should be easy to hook up a save_attempt function and add a 3rd button into the modal (flash / send / attempt) as written here
  • For the bids and attempts in the api we currently use the same value, because it's unclear what fields are being used. But so far comments, tries, rating, comments, emojis, etc. are all shown correctly in the aurora apps.
image

Copy link
Owner

@lemeryfertitta lemeryfertitta left a comment

Choose a reason for hiding this comment

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

Awesome! I played with this briefly but generally seems like a really awesome feature addition. Had some minor comments on the functionality with my quick test drive, but mostly looking good to me. I'll spend a bit more time playing around with it before we get it merged. Just a few comments on code quality items, and obviously we need to get the boardlib updates in first, but would be happy to get this merged and deployed soon!

Thanks @gardaholm!

climbdex/api.py Outdated Show resolved Hide resolved
climbdex/static/js/results.js Outdated Show resolved Hide resolved
climbdex/static/js/results.js Outdated Show resolved Hide resolved
climbdex/static/js/results.js Outdated Show resolved Hide resolved
climbdex/templates/results.html.j2 Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@gardaholm
Copy link
Contributor Author

updated aggregated_logbook() to use the aggregate=True parameter for the modified logbook_entries as written in this PR lemeryfertitta/BoardLib#41

Karlosschmidtos and others added 3 commits August 22, 2024 11:53
using bid_count instead of attempt_id to log attempts.
updated view.py to handle login in via user_id/token only
@@ -9,7 +9,9 @@ function drawClimb(
frames,
Copy link
Owner

@lemeryfertitta lemeryfertitta Aug 26, 2024

Choose a reason for hiding this comment

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

I tried updating boardlib to 0.7.1 and running this PR locally. There were a number of things that were broken for me:

  • Upon successful login there is no indication of success (the login text doesn't change) and climbs don't appear to be ticked. Seemed like logging in was generally broken, but I didn't see any error messages in the console or logs.
  • Board image is overflowing, at least for Kilter 8x12
image

Let me know if you are experiencing the same or if I'm doing something wrong!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strange, didn't happen on our tests here, but I will spin up our test version on fly.io with the official boardlib 0.7.1 and will check it. will probably be able to do it tomorrow morning. sorry for that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you sure it installed the new requirements?
I've spinned a fly.io version here https://climbdex-wandering-moon-444.fly.dev/ (linked to boardlib 0.7.1)
and it seems to work for me …

  • tested kilter + tension board with my aurora credentials
  • I could login, and get the different status message (You're logged in!)
  • can see my ticked and tried climbs (green + yellow)
  • could log my climb (logged climbs only get displayed after a full refresh, when the logbook is renewed), also get the Successfull Tick Message.
  • the problem with the kilter and tension board classic layout can be fixed via css (will add a commit, seems to be a overflow problemon certain screen dimensions)

Copy link
Owner

@lemeryfertitta lemeryfertitta Aug 27, 2024

Choose a reason for hiding this comment

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

Ah, I found the issue. The local deployment bind I was running on 0.0.0.0:8000 (was doing it for some mobile testing) didn't allow the cookie to be stored, so it just silently was trying and failing to set the cookie. Created #83 for that, as it's pretty confusing why the login doesn't work if your cookies aren't enabled, but it is unrelated to your PR here. As an aside, I figured out how to close the modal without refreshing the whole page (your location.reload change), but I'll just update that in a separate PR.

I think the only thing that is a regression from this PR is the overflow issue. I think its just some issue with the bootstrap containers that were changed to add the new buttons and such.

Also, I just played with the tick feature a little bit more and had a few additional thoughts:

  1. Ticking repeats is a little bit weird, it looks identical to trying the climb over multiple sessions and only sending once vs. sending in multiple sessions. I wonder if we shouldn't be trying to aggregate here, and we should just show a list of sessions for each climb. So basically this would just be the list of all of the logbook entries from boardlib for a specific climb

  2. Maybe we can rename "Statistics" to "Your Attempts" or something similar. I feel like when you click statistics you expect to get the same information as the info button on the official apps, which shows other peoples ticks and grade distribution chart and such.

We can leave 1. as is for now and change that in a future PR, I think it's worth just getting the current functionality in. Let's get the overflow issue fixed and rename the menu item for this PR.

Copy link
Contributor Author

@gardaholm gardaholm Aug 27, 2024

Choose a reason for hiding this comment

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

I think the only thing that is a regression from this PR is the overflow issue. I think its just some issue with the bootstrap containers that were changed to add the new buttons and such.

should be fixed. I think the occurence is pretty rare >> probably 13'' notebooks + board images which are very long.

Also, I just played with the tick feature a little bit more and had a few additional thoughts:

  1. Ticking repeats is a little bit weird, it looks identical to trying the climb over multiple sessions and only sending once vs. sending in multiple sessions. I wonder if we shouldn't be trying to aggregate here, and we should just show a list of sessions for each climb. So basically this would just be the list of all of the logbook entries from boardlib for a specific climb

true. I was thinking about the same: if you open the log item a small indication about the sessions would be great and if there are already sessions logged then the flash button can be hidden (I already ran into this issue a few times and wondering why the app don't show the flash icon …). Information is already there so I guess it shouldn't be too much work for a future PR.

What this PR is also missing is a indication that the current climb is logged (besided the success message). It would be nice if the entry in the result list gets the background color and the tick icon after saving the modal. Shouldn't be too hard, maybe I can do this in another PR, it's more a cosmetical thing I guess …

  1. Maybe we can rename "Statistics" to "Your Attempts" or something similar. I feel like when you click statistics you expect to get the same information as the info button on the official apps, which shows other peoples ticks and grade distribution chart and such.

also fine, renamed the entries (Menu + Modal-Title).
Would love to have the stats. grad distribution and comment also somewhere in climbdex. but probably something for later.

Copy link
Owner

Choose a reason for hiding this comment

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

should be fixed. I think the occurence is pretty rare >> probably 13'' notebooks + board images which are very long.

Didn't really like the overflow approach so I changed it back to the original behavior to scale based on viewport. Also made a few other minor changes to some of the nomenclature but will get this merged and deployed now! Lots of other improvements to make but its a huge start 😄

fixed overflow for #div-climb on results and create-climb page.
renamed menu item and modal title
gardaholm and others added 3 commits August 27, 2024 20:36
mistakenly removed id="div-climb" in the previous commit. added it back as before …
@lemeryfertitta lemeryfertitta merged commit 3636389 into lemeryfertitta:main Aug 29, 2024
@gardaholm
Copy link
Contributor Author

Thanks for the merge and your work! Hope its a good first addition for the save dialog and we could save you at least some time :)
and thanks for refining the code 🙏

@ItsDang ItsDang mentioned this pull request Sep 2, 2024
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