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.

Sign up for GitHub

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

Jump to bottom

Enh(Message): Component splitting and store migration (Reactions only) #11261

Merged
merged 18 commits into from
Jan 3, 2024
Merged

Enh(Message): Component splitting and store migration (Reactions only) #11261

merged 18 commits into from
Jan 3, 2024

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Dec 18, 2023 •
edited
Loading

🚧 Tasks

  • Migration of reactions to a new component
  • Store migration
  • Fix reactions update with receiving messages
  • Fix reactions count and reacting participants list synchronization
  • Patch reactions
  • Tests 😿

Note: 'reaction' system message is not coming instantly like other reaction related system messages and sometimes is not even sent and reactions gets refetched upon the regular chat fetch. That's why you can notice that reactions come all at once as a batch when more than one is sent.

  • Bug reported: counter is not in sync with reacting participants list
  • Follow-up: patch reactions

🏁 Checklist

@DorraJaouad DorraJaouad added this to the 💞 Following Major (29) milestone Dec 18, 2023
@DorraJaouad DorraJaouad self-assigned this Dec 18, 2023
@DorraJaouad DorraJaouad requested a review from Antreesy December 20, 2023 12:33
@DorraJaouad DorraJaouad force-pushed the enh/noid/refactor-message branch from 1be3ca0 to 8ff714a Compare December 21, 2023 10:12
Antreesy
Antreesy reviewed Dec 21, 2023
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Here goes a first part, with moving the component:

src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/Message.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/Message.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/Message.vue Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Show resolved Hide resolved
@DorraJaouad DorraJaouad force-pushed the enh/noid/refactor-message branch from 628cdeb to 40c9853 Compare December 21, 2023 12:50
Antreesy
Antreesy reviewed Dec 21, 2023
View reviewed changes
src/store/messagesStore.js Outdated Show resolved Hide resolved
src/store/messagesStore.js Outdated Show resolved Hide resolved
Antreesy
Antreesy reviewed Dec 26, 2023
View reviewed changes
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/store/messagesStore.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
@DorraJaouad DorraJaouad force-pushed the enh/noid/refactor-message branch 3 times, most recently from 272559d to dc0c4c4 Compare December 27, 2023 20:34
@DorraJaouad DorraJaouad marked this pull request as ready for review December 27, 2023 22:01
Antreesy
Antreesy reviewed Dec 28, 2023
View reviewed changes
src/store/messagesStore.spec.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Show resolved Hide resolved
src/store/messagesStore.js Outdated Show resolved Hide resolved
@DorraJaouad DorraJaouad force-pushed the enh/noid/refactor-message branch 2 times, most recently from 426278c to 6e95640 Compare December 30, 2023 21:08
Antreesy
Antreesy reviewed Dec 31, 2023
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Last one hopefully, just noticed some thing to optimize / fix

src/store/conversationsStore.js Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.spec.js Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.spec.js Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.spec.js Outdated Show resolved Hide resolved
src/stores/__tests__/reactions.spec.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Outdated Show resolved Hide resolved
src/stores/reactions.js Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.vue Outdated Show resolved Hide resolved
@DorraJaouad DorraJaouad mentioned this pull request Dec 31, 2023
Closed
Antreesy
Antreesy reviewed Jan 2, 2024
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Apart from that and hasOutdatedDetails, see nothing blocking

Don't forget to rebase and resolve conflicts

src/components/MessagesList/MessagesGroup/Message/MessagePart/Reactions.spec.js Outdated Show resolved Hide resolved
@DorraJaouad DorraJaouad force-pushed the enh/noid/refactor-message branch from b62334b to 6db1aa2 Compare January 2, 2024 20:05
@DorraJaouad
Remove extra parameter
c84eec2 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Reaction store fixes to reduce fetching of details
bb03d89 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Add tests for reactions store
c6b094a 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Move reaction tests to its corresponsing components tests suite
3d1eb23 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Purge reactions store when chat history is deleted or conversation is…
fe2005f 
… deleted

Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Adjust messagesStore tests
136ab08 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Add loading state for reactions details
3c158dd 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Move reaction actions to reactionsStore
0fe30c7 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Improve Reactions.vue coverage
23a8cf0 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Some fixes
db9faf0 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad
Fix(Reactions): add dynamic order by count
4b29193 
Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad DorraJaouad force-pushed the enh/noid/refactor-message branch from 6db1aa2 to 4b29193 Compare January 3, 2024 12:45
Antreesy
Antreesy approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested locally and against c.nc

Let's split it task by task next time 🙈

@DorraJaouad DorraJaouad merged commit 0278593 into main Jan 3, 2024
36 checks passed
@DorraJaouad DorraJaouad deleted the enh/noid/refactor-message branch January 3, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Antreesy Antreesy Antreesy approved these changes

Assignees

@DorraJaouad DorraJaouad

Labels
feature: frontend 🖌️ "Web UI" client technical debt
Projects
None yet
Milestone
v19.0.0-beta.1
Development

Successfully merging this pull request may close these issues.
2 participants
@DorraJaouad @Antreesy