-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: level up summary #2182
feat: level up summary #2182
Conversation
@ggurdin Ready for review and test, staging choreo is accepting the requests made in this PR. |
@WilsonLe When passing the choreographer response to the dialog, is there a reason to pass the matrix event info (the eventID and roomID) rather than passing the choreographer response directly? Adding the extra step of waiting until the event is sent to the room and then going and fetching the event from the room takes a little extra time (I think it's negligible though), but also opens the door for more potential errors (i.e., I'm not sure exactly what happened, but while testing I got a good response from the choreographer, but the "View Summary" button was missing. Maybe the analyticsRoomID was null for some reason, or the event hadn't been sent yet). Would it be possible to pass the choreo response directly to the dialog? We would still send the analytics state event, we just wouldn't wait for it to go through. |
There was an initial delay of around 500ms to show the level up popup. I removed that and did all the async calls to mimic that 500ms. I mean there is way to forward the choreo response but then it wouldn't have the same delay as the 500ms we had before this. |
@WilsonLe My main point is that it would be less error-prone and require one less call to the server to get the state event. |
Makes sense. Though I'm planning to reuse the summary popup somewhere else for user to revisit their reports for previous levels. I'll add an optional param to the widget that the choreo response is passed directly to. If that optional param is missing then we make another synapse call. Of course, the first and last construct summary is called for that current level, then open the popup with the new optional param defined. |
@WilsonLe Sounds good to me! |
@WilsonLe Besides that I think this is good to merge! I really like the summaries. |
Got it. I might add a max width to the popup too. |
@ggurdin ready for another round of review |
Looks good to me :^) |
When user levels up, they should have the ability to view their summary of construct uses for the level they just went up from.
This PR is still a draft PR since it still relies on a choreographer endpoint, the progress of which is tracked here https://github.com/pangeachat/2-step-choreographer/issues/506
construct-use-summary.mov