-
Notifications
You must be signed in to change notification settings - Fork 1
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
Bug fix: user avatar does not update on challenge completion #348
Conversation
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.
Thank you for fixing this bug !
This effectively contributes to making our application more responsive to the user and so improves the overall UX.
Your code is of great quality with abondant documentation 👍
The PR overall looks good to me and I saw that you added some tests for the new view model which is great.
I have just made a few suggestion below.
Thanks for your great work 🎉
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.
Thank you for bringing the modifications !
The PR looks good to me now 👍
Amazing job 🎉
Thank you, @yoannLafore, for your review 🎉 As this PR changes a lot of things, we are going to wait for a second reviewer before merging 💯 (@JoachimFavre) |
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.
Good job with this great PR! I have some comments, don't be afraid of their number their number is simply proportional to the size of your PR (and, they should be quick to fix!)
I just have another general comment. I feel like you no longer a DynamicUserAvatar
and a UserAvatar
, we can get rid of DyanmicUserAvatar
, can't we?
test/mocks/overrides/override_user_centauri_points_view_model.dart
Outdated
Show resolved
Hide resolved
You are completely right ! I answered in this comment: #348 (comment) |
This commit moves the user avatar color refreshing and the fetching of centauri points in the dynamic user avatar view model
As I changed a big chunk since the approval of @yoannLafore, I think he should take a look at the new code. |
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 have gone through the resolved conversations and they look good to me 👍
I also learned a few things that I didn't know about dart.
Thanks for your great work @CHOOSEIT and @JoachimFavre 🎉
The PR looks good to 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.
You made an amazing job at fixing my comments, I don't have anything to say on the code you modified. Great job!
I'm sorry for the refactor to move the points refresh to the dynamic profile view-model, I have to admit I did not really anticipate it would be so big. I however find your code absolutely incredible after this refactor, so thank you so much for doing it! :)
LGTM!
Fixes: #342
This PR aims to fix the user avatar not updating properly on challenge completion.
As you can see with the line changes, this was not a straight-forward bug fix.
We had not any updates on the user avatars. The user avatars were also not linked to any provider, hence, on challenge completion, we had no way to update every user avatar at once.
This PR creates a new provider
userCentauriPointsViewModelProvider
, that gets the centauri points of a user given its user id. This provider is refreshable, allowing us to watch on it in every user avatar.Note that the user avatar had not access to the user id of the displayed user, so a considerable part of this PR is giving this access to this information.