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

fix(friends-requests): improved requests and notifications display #3955

Merged
merged 8 commits into from
Jul 19, 2022

Conversation

ThomBos
Copy link
Contributor

@ThomBos ThomBos commented Jul 12, 2022

What this PR does 📖
Fixed friends requests display
Which issue(s) this PR fixes 🔨
AP-1954

Special notes for reviewers 🗒️

Additional comments 🎤
Still missing some data to show (name and image, just when the other user hasn't accepted the request)

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Jul 12, 2022
@josephmcg josephmcg added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Jul 14, 2022
@ThomBos ThomBos added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Jul 15, 2022
@ThomBos
Copy link
Contributor Author

ThomBos commented Jul 15, 2022

@josephmcg let me know what you think about these changes! 🔨

Copy link
Contributor

@josephmcg josephmcg left a comment

Choose a reason for hiding this comment

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

a couple ideas regarding FriendsManager state structure. If we switched details and requests to arrays, it would simplify a lot of this logic and improve Vue reactivity

Comment on lines +31 to +33
iridium.friends?.on('request/error', (err) => {
this.error = err
})
Copy link
Contributor

@josephmcg josephmcg Jul 19, 2022

Choose a reason for hiding this comment

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

would it be simpler to wrap the error creating methods in a try/catch (or promise.catch if async)?

@josephmcg josephmcg added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Jul 19, 2022
@ThomBos
Copy link
Contributor Author

ThomBos commented Jul 19, 2022

Quick recap:

  • all states are now arrays instead of objects, removed details state because it's now stored into list
  • requestSave clears non-pending requests and sends requests change events using requestSendUpdate
  • removeFriend is disconnected from requestSendUpdate (but it does basically the same thing) because 'accepted' or 'rejected' requests are no more stored in the state
  • removed unused functions

Errors management is still in progress and has to be improved. 🔨
At the moment i'm thinking of adding a new state to contain an error string, so the components can render as it changes.
This would cause all "throw" to be removed, in order to remove all try/catches around the app...

Any suggestion to improved this code are well welcomed!! 💥🚀

@molimauro molimauro added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Jul 19, 2022
@ThomBos ThomBos force-pushed the AP-1954 branch 2 times, most recently from 8e6bac3 to 05138e1 Compare July 19, 2022 13:37
@WanderingHogan WanderingHogan dismissed josephmcg’s stale review July 19, 2022 15:32

we can come back to smaller things like this on cleanup, this pr is a good one to build off of

@molimauro molimauro merged commit 84fcabb into iridium-dev Jul 19, 2022
@molimauro molimauro deleted the AP-1954 branch July 19, 2022 16:09
@github-actions github-actions bot removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Jul 19, 2022
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.

4 participants