Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Tick Climbs and Show Stats #72
Changes from all commits
12dd4c5
2507028
d7a1b92
d254fd0
da4a5d0
f8e7d5b
4124ef3
3869a2c
531419b
52b55a1
4e5d72d
d3c6d5e
9d7f553
c32723c
c9417e5
60dbe88
8d99168
9aaf434
3d7aa68
c34e747
e732edf
b7be947
a687ce7
6a6e155
1a35c2b
582f5bb
9854444
3656539
f150117
cf3a7ee
bdc521c
f4c7b51
cb94194
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:
Let me know if you are experiencing the same or if I'm doing something wrong!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 …
There was a problem hiding this comment.
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 (yourlocation.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:
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
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.
There was a problem hiding this comment.
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.
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 …
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄